search

withastro / starlight

🌟 Build beautiful, accessible, high-performance documentation websites with Astro
https://starlight.astro.build
MIT License
4.98k stars 534 forks source link

Synced tabs #462

Closed HiDeoo closed 1 year ago

HiDeoo commented 1 year ago

What version of starlight are you using?

0.6.1

What is your idea?

This proposal is a follow-up on a Discord discussion about the addition of synced tabs to Starlight.

Why is this feature necessary?

This features is pretty useful to show tabular data where the user will benefit from having the tabs synced. e.g.:

Do you have examples of this feature in other projects?

https://github.com/withastro/starlight/assets/494699/5b92c6b3-0d79-4958-ae27-68fbba16d6d8

https://github.com/withastro/starlight/assets/494699/64a11c1c-3ba1-4262-8416-5aca5ca34f46

https://github.com/withastro/starlight/assets/494699/bf44cbc3-7051-46fd-9a4a-16592c149610

API

I like the Astro docs current approach of basically passing a basic string to the <Tabs /> component when you want to sync the tabs. This is a very simple API and it is easy to understand what is going on imo.

Altho, I am personally not a huge fan of the sharedStore prop name currently used. It kinda feels like it is leaking some implementation details and not super obvious it's related to syncing the tabs. I would prefer something like sync or syncKey for example.

Any other ideas for the end user API?

Implementation

The Astro current implementation is using a Preact component with a hook to handle the sync which relies on Nano Stores to sync some state between the tabs. I would assume this is an approach we would want to avoid in Starlight as it would add some dependencies and one UI framework.

My component starlight-package-managers is using some MutationObserver to detect tab changes and update the selected tab accordingly. This choice is mostly due to the fact that I am doing that outside of Starlight and it is not a good choice for Starlight itself.

@delucis mentioned using a simple store primitive that each tabs instance subscribes to and updates from. This is a solid idea, we could expose a relatively small createStore() function returning a store with various methods like store.getState(), store.subscribe() and store.setState() for example. Later down the line we could even imagine some other components re-using this store primitive to sync some other state (altho I don't see an example right now).

Altho, after thinking about it for a bit, I wonder if right now, the simplest approach would not be to shove everything in the existing StarlightTabs component as I don't think we need that much to implement this feature. We could have a static property with a record of all the tabs instances initialized with a sync key keyed by these keys and a static method which would be called when a new tab is selected that would update the selected tab for all the tabs instances with the same sync key.

A quick diff could look like this (note that is a very rough draft, not properly written or thought, tested or anything):

diff --git a/packages/starlight/user-components/Tabs.astro b/packages/starlight/user-components/Tabs.astro
index 43af043..d4a1f38 100644
--- a/packages/starlight/user-components/Tabs.astro
+++ b/packages/starlight/user-components/Tabs.astro
@@ -1,11 +1,16 @@
 ---
 import { processPanels } from './rehype-tabs';

+interface Props {
+   syncKey?: string;
+}
+
+const { syncKey } = Astro.props;
 const panelHtml = await Astro.slots.render('default');
 const { html, panels } = processPanels(panelHtml);
 ---

-<starlight-tabs>
+<starlight-tabs data-sync-key={syncKey}>
    {
        panels && (
            <div class="tablist-wrapper not-content">
@@ -70,8 +75,11 @@ const { html, panels } = processPanels(panelHtml);

 <script>
    class StarlightTabs extends HTMLElement {
+       static #syncedTabs: Record<string, StarlightTabs[]> = {};
+
        tabs: HTMLAnchorElement[];
        panels: HTMLElement[];
+       #syncKey: string | undefined;

        constructor() {
            super();
@@ -79,6 +87,14 @@ const { html, panels } = processPanels(panelHtml);
            this.tabs = [...tablist.querySelectorAll<HTMLAnchorElement>('[role="tab"]')];
            this.panels = [...this.querySelectorAll<HTMLElement>('[role="tabpanel"]')];

+           this.#syncKey = this.dataset.syncKey;
+
+           if (this.#syncKey) {
+               const syncedTabs = StarlightTabs.#syncedTabs[this.#syncKey] ?? [];
+               syncedTabs.push(this);
+               StarlightTabs.#syncedTabs[this.#syncKey] = syncedTabs;
+           }
+
            this.tabs.forEach((tab, i) => {
                // Handle clicks for mouse users
                tab.addEventListener('click', (e) => {
@@ -113,7 +129,19 @@ const { html, panels } = processPanels(panelHtml);
            });
        }

-       switchTab(newTab: HTMLAnchorElement | null | undefined, index: number) {
+       static #syncTabs(emitter: StarlightTabs, index: number) {
+           const syncKey = emitter.#syncKey;
+           if (!syncKey) return;
+           const syncedTabs = StarlightTabs.#syncedTabs[syncKey];
+           if (!syncedTabs) return;
+
+           for (const tab of syncedTabs) {
+               if (tab === emitter) continue;
+               tab.switchTab(tab.tabs[index], index, false);
+           }
+       }
+
+       switchTab(newTab: HTMLAnchorElement | null | undefined, index: number, sync = true) {
            if (!newTab) return;

            // Mark all tabs as unselected and hide all tab panels.
@@ -131,7 +159,10 @@ const { html, panels } = processPanels(panelHtml);
            // Restore active tab to the default tab order.
            newTab.removeAttribute('tabindex');
            newTab.setAttribute('aria-selected', 'true');
-           newTab.focus();
+           if (sync) {
+               newTab.focus();
+               StarlightTabs.#syncTabs(this, index);
+           }
        }
    }

Any thoughts on this? Some other ideas?

Testing

If I am not mistaken, we are currently not testing any of the Starlight components. Do we want to add tests for this? If yes, what approach would we want to take? Spinning up something like Playwright to test the components in a browser could add a lot of confidence in this but may feels a bit too much just for this feature. Another approach as we are using vitest already could be switching the test environment from node to happy-dom for example for a new set of tests, extract the web component from the Astro component (like StarlightTOC) and figure out how to test it using happy-dom. I have never done that for web components but I would assume it is possible. Any thoughts on this?

Other

Did I miss anything? ^^

Participation

lorenzolewis commented 1 year ago

Any thoughts on tab-syncing being the default option? Use the tab label prop and if a tab is switched, switch any other tabs that share that same label. You can have a bool to opt-out of this default behaviour.

Another option could be to derive a key from all the labels used within a Tabs group and then if a "group" is exactly the same opt them into syncing.

For example you have a <Tabs> group with the following <TabItem> label's: npn, Yarn, pnpm. If another <Tabs> group has those same exact values for its children then opt-in to syncing.

No idea is these are good ideas instead of requiring opt-in, but figured I'd throw them out there 😊

HiDeoo commented 1 year ago

Any thoughts on tab-syncing being the default option?

Did not think of that, thanks for sharing. I usually prefer to opt-in to feature but in this case, I don't think I can think of some bad side effects, specially if you can opt out (I imagine these cases would be rare) :thinking:

Maybe some other people have some ideas or reasons why it should be or not be the default?

Sporiff commented 1 year ago

Coming from the world of MyST markdown and sphinx-design, I would personally prefer opting in with a specific sync directive because it gives me control over which tabs I synchronize. The only situation in which it is automatic in MyST is when you're writing out a set of code blocks to make examples.

:::{tab-set-code}

```json
{
   "total": 1,
   "page": 1,
   "per_page": 5,
   "subscriptions": [
      {
         "feed_url": "https://example.com/rss1",
         "guid": "64c1593b-5a1e-4e89-b8a3-d91501065e80",
         "is_subscribed": true,
         "guid_changed": "2022-12-23T10:24:14.670Z",
         "new_guid": "36a47c4c-4aa3-428a-8132-3712a8422002"
      }
   ]
}
<?xml version="1.0" encoding="UTF-8"?>
<subscriptions>
    <total>1</total>
    <page>1</page>
    <per_page>5</per_page>
    <subscription>
        <feed_url>https://example.com/rss1</feed_url>
        <guid>64c1593b-5a1e-4e89-b8a3-d91501065e80</guid>
        <is_subscribed>true</is_subscribed>
        <guid_changed>2022-12-23T10:24:14.670Z</guid_changed>
        <new_guid>36a47c4c-4aa3-428a-8132-3712a8422002</new_guid>
    </subscription>
</subscriptions>

:::


This would generate a set of tabs that have `sync` values of `tabcode-json` and `tabcode-xml` respectively. A writer could choose to sync any other tab to these.

````markdown
::::{tab-set}
:::{tab-item} JSON
:sync: tabcode-json

```bash
curl --location '/subscriptions' \
--header 'Content-Type: application/json' \
--data '{
  "subscriptions": [
    {
      "feed_url": "https://example.com/feed1"
    },
    {
      "feed_url": "https://example.com/feed2"
    },
    {
      "feed_url": "https://example.com/feed3"
    },
    {
      "feed_url": "example.com/feed4",
      "guid": "2d8bb39b-8d34-48d4-b223-a0d01eb27d71"
    }
  ]
}'

::: :::{tab-item} XML :sync: tabcode-xml

curl --location '/subscriptions' \
--header 'Content-Type: application/xml' \
--data '<?xml version="1.0" encoding="UTF-8"?>
<subscriptions>
    <subscription>
        <feed_url>https://example.com/feed1</feed_url>
    </subscription>
    <subscription>
        <feed_url>https://example.com/feed2</feed_url>
    </subscription>
    <subscription>
        <feed_url>https://example.com/feed3</feed_url>
    </subscription>
    <subscription>
        <feed_url>example.com/feed4</feed_url>
        <guid>2d8bb39b-8d34-48d4-b223-a0d01eb27d71</guid>
    </subscription>
</subscriptions>'

::: ::::

delucis commented 1 year ago

Thanks for the super detailed write-up @HiDeoo!

I like all the ideas here I think. I’d be plus one on something like syncKey or syncId as the attribute to opt in to this. Opt-in feels right to me here and I generally find opt-in APIs much easier to understand and document as well.

I wrote a tiny “atom” store while working on the theme editor id that could be helpful?

https://github.com/withastro/starlight/blob/d7f2ab405b90a0c47b4b1899f91da0420b990fac/docs/src/components/theme-designer/atom.ts#L1-L20

But I’m also fine with whatever solution you think feels best — keeping it in the tabs class is fine by me!

Re: testing — some testing would indeed be cool. I don’t have strong opinions here. The drawback of only testing the custom element code with Vitest and a browser environment would be that we aren’t testing the markup generated by the Astro component, so could only give us a certain amount of confidence. We could also wait it out as PRs like https://github.com/withastro/astro/pull/7979 will start to bring better testing support for Astro stuff closer too I’d say.