Skip to content

Commit

Permalink
feat(web): TabGroup reliablity changes, uncloseable tabs
Browse files Browse the repository at this point in the history
A couple pathological behaviors can happen with the TabGroup that this
commit attempts to resolve:

  - The `TabGroupItem` elements are always mounted *after* the
    `TabGroup` itself, and may be mounted much later than other reactive
    portions of the page. This means that the call to `selectTab` can
    fail because the `TabGroupItem` has not yet had a chance to call
    `registerTab` on the TabGroup ref. To solve this I've added a ref
    that tracks a failed selectTab. If that tab is registered after the
    select call, and no other tab selection has occurred, we
    automatically select the "pending" tab. For the AssetEditorTabs this
    also means that calls to `nextTick` are unnecessary since the
    missing tab will just be marked as pending and will be selected when
    the `registerTab` call succeeds.

  - Teleports into other components rendered by Vue (instead of to
    portions of the DOM not handled by Vue) are unreliable. Consider the
    following chain of events:

      1. a TabGroup is rendered.
      2. the TabGroup items are rendered, and the active one attempts to
         render itself via a teleport.
      3. Before that happens, the TabGroup itself is re-rendered
         for some reason.
      4. At that point in time, the teleport target might not be present
         in the DOM, and the Teleport will fail (I saw this often while
         building the AssetEditorTabs), crashing the frontend JS
         execution.

    While in some cases this happens because too many renders are
    occuring, it's still possible for it to happen via perfectly normal
    rendering paths, for example if you switch from one Asset to the
    next one before one of the TabGroupItem teleports has had the chance
    to teleport into the (now destroyed) TabGroup. `vue-safe-teleport`
    solves this by providing a teleport target that is queried for its
    ready state, and only mounting the `Teleport` element when the
    target is ready.

    See vuejs/core#2015 for details on this
    issue.

This also adds the ability to mark a single tab as uncloseable if the
tab group has closeable = true.
  • Loading branch information
zacharyhamm committed Jun 12, 2023
1 parent 7cb2244 commit 991aaf5
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 15 deletions.
3 changes: 2 additions & 1 deletion app/web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@
"vanilla-picker": "^2.12.1",
"vue": "^3.3.4",
"vue-konva": "^3.0.1",
"vue-router": "^4.1.6"
"vue-router": "^4.1.6",
"vue-safe-teleport": "^0.1.2"
},
"devDependencies": {
"@iconify/json": "^2.1.142",
Expand Down
3 changes: 3 additions & 0 deletions app/web/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { createApp } from "vue";
import FloatingVue from "floating-vue";
import VueKonva from "vue-konva";
import { createHead } from "@vueuse/head";
import VueSafeTeleport from "vue-safe-teleport";

import "@si/vue-lib/tailwind/main.css";
import "@si/vue-lib/tailwind/tailwind.css";
Expand All @@ -24,4 +25,6 @@ app.use(FloatingVue, { container: "#app-layout" });
// TODO: fork the lib and set it up so we can import individual components
app.use(VueKonva);

app.use(VueSafeTeleport);

app.mount("#app");
32 changes: 26 additions & 6 deletions lib/vue-lib/src/design-system/tabs/TabGroup.vue
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
</template>
<template v-else>{{ tab.props.label }}</template>
<button
v-if="closeable"
v-if="closeable && !tab.props.uncloseable"
class="inline-block rounded-full text-neutral-400 ml-1"
:class="
clsx(
Expand Down Expand Up @@ -108,7 +108,7 @@
</div>

<!-- the tabgroup item uses a teleport to render its default slot content here if active -->
<div :id="teleportId" class="overflow-auto flex-grow relative" />
<TeleportTarget :id="teleportId" class="overflow-auto flex-grow relative" />
</template>
</div>
</template>
Expand All @@ -119,6 +119,7 @@ type TabGroupContext = {
registerTab(id: string, component: TabGroupItemDefinition): void;
unregisterTab(id: string): void;
selectTab(id?: string): void;
tabExists(id?: string): boolean;
teleportId: string;
};
Expand Down Expand Up @@ -208,6 +209,10 @@ function registerTab(slug: string, component: TabGroupItemDefinition) {
autoSelectTab(true);
});
}

if (pendingTabSlug.value && pendingTabSlug.value === slug) {
selectTab(slug);
}
}
function unregisterTab(slug: string) {
if (unmounting.value) return;
Expand All @@ -229,6 +234,11 @@ function refreshSettingsFromTabs() {
// currently there are no settings here - any child settings to set on the parent would go here
}

function tabExists(slug?: string) {
return !!(slug && tabs[slug]);
}

const pendingTabSlug = ref<string | undefined>();
const lastSelectedTabIndex = ref(0);
function selectTab(slug?: string) {
if (unmounting.value) return;
Expand All @@ -241,8 +251,16 @@ function selectTab(slug?: string) {
}

// select the tab
if (slug && tabs[slug]) selectedTabSlug.value = slug;
else selectedTabSlug.value = undefined;
if (slug && tabs[slug]) {
selectedTabSlug.value = slug;
pendingTabSlug.value = undefined;
} else {
// If the tab is not yet present, we mark this as the pending tab slug. When
// registerTab is called with a matching slug, that tab will be selected.
// Any other tab selection clears the pending tab slug
selectedTabSlug.value = undefined;
pendingTabSlug.value = slug;
}

lastSelectedTabIndex.value = _.indexOf(
orderedTabSlugs.value,
Expand Down Expand Up @@ -279,7 +297,7 @@ function selectTab(slug?: string) {
);
}
}
}
}

// emit new selected tab to parent in case it needs it, for example to sync the URL
emit("update:selectedTab", selectedTabSlug.value);
Expand All @@ -294,6 +312,7 @@ const rememberLastTabStorageKey = computed(() => {
});

function autoSelectTab(isInitialSelection = false) {
pendingTabSlug.value = undefined;
if (isNoTabs.value) {
// can't select anything if there are no tabs
// selectTab();
Expand Down Expand Up @@ -379,9 +398,10 @@ const context = {
registerTab,
unregisterTab,
selectTab,
tabExists,
teleportId,
};
provide(TabGroupContextInjectionKey, context);

defineExpose({ selectTab });
defineExpose({ selectTab, tabExists });
</script>
7 changes: 4 additions & 3 deletions lib/vue-lib/src/design-system/tabs/TabGroupItem.vue
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

<!-- if tab is selected, teleport default slot into the main tab content area -->
<template v-if="slug === menuCtx.selectedTabSlug.value">
<Teleport :to="`#${menuCtx.teleportId}`">
<SafeTeleport :to="`#${menuCtx.teleportId}`">
<slot />
</Teleport>
</SafeTeleport>
</template>
</template>

Expand All @@ -18,7 +18,7 @@ import { useTabGroupContext } from "./TabGroup.vue";
import type { Slot } from "vue";

export type TabGroupItemDefinition = {
props: { slug: string; label: string };
props: { slug: string; label: string; uncloseable: boolean };
slots: {
default?: Slot;
label?: Slot;
Expand All @@ -29,6 +29,7 @@ export type TabGroupItemDefinition = {

const props = defineProps({
label: { type: String },
uncloseable: { type: Boolean, default: false },
slug: { type: String, default: () => `tab-group-${idCounter++}` },
});

Expand Down
63 changes: 58 additions & 5 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 991aaf5

Please sign in to comment.