Fix tab group sync #8483

0916dhkim · 2022-12-25T20:37:06Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Fixing #8473

Test Plan

Test Syncing tab choices section of Docusaurus Tabs page. Syncing feature should work as expected.
Test Query string section of the same page. "Android, iOS" tab group should be synced and URL query params should change on select.

Test links

Deploy preview: https://deploy-preview-8483--docusaurus-2.netlify.app/

Related issues/PRs

fix #8473

#8225

netlify · 2022-12-25T20:41:26Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`c44e113`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/63a8b4743c958b0008f8a906
😎 Deploy Preview	https://deploy-preview-8483--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-12-25T20:42:00Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟢 94	🟢 97	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 79	🟢 100	🟢 100	🟢 100	🟢 90	Report

0916dhkim

Changes Summary

Current implementation does not update selectedValue when there is a query parameter or local storage change (only restores on mount). That's why tab group sync is not working.

Because selected tab is stored in React state, local storage, and query params, I thought it would be helpful to manage all three in tabGroupChoice.tsx module.

New structure

const {selectedValue, setTabGroupChoice} = useTabGroupChoice(...);

setTabGroupChoice updates selectedValue, local storage (if groupId is given), and query params (if queryString is given).
When a value in local storage or query params changes, selectedValue is updated to match that value. This is the mechanism of syncing multiple tabs in a group.

0916dhkim · 2022-12-25T20:42:43Z

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx

  const defaultValue =
    defaultValueProp !== undefined
      ? defaultValueProp
      : children.find((child) => child.props.default)?.props.value ??
        children[0]!.props.value;
-
-  const [selectedValue, setSelectedValue] = useState(defaultValue);


Manage selectedValue state inside useTabGroupChoice hook.

0916dhkim · 2022-12-25T20:44:17Z

packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx

      blockElementScrollPositionUntilNextRender(newTab);
-      setSelectedValue(newTabValue);


I thought of moving blockElementScrollPositionUntilNextRender into tabGroupChoice.tsx, but decided to leave it as-is. Let me know if you think it's better to move it.

0916dhkim · 2022-12-25T20:45:35Z

packages/docusaurus-theme-common/src/contexts/tabGroupChoice.tsx

-  /** A boolean that tells if choices have already been restored from storage */
-  readonly ready: boolean;
-  /** A map from `groupId` to the `value` of the saved choice. */
-  readonly tabGroupChoices: {readonly [groupId: string]: string};
+  /** A map from `groupId` to the `value` of the saved choice in storage. */
+  readonly tabGroupChoicesInStorage: {readonly [groupId: string]: string};
+  /** A map from `searchKey` to the `value` of the choice in query parameter. */
+  readonly tabGroupChoicesInQueryParams: {readonly [searchKey: string]: string};


Remove ready property.

Manage both local storage values & query param values in this context.

0916dhkim · 2022-12-25T20:47:04Z

packages/docusaurus-theme-common/src/contexts/tabGroupChoice.tsx

-    (groupId: string, newChoice: string) => {
-      setChoices((oldChoices) => ({...oldChoices, [groupId]: newChoice}));
-      setChoiceSyncWithLocalStorage(groupId, newChoice);
+  const setTabGroupChoice = useCallback(


setTabGroupChoice pushes tab choice to local storage or query param depending on groupId & queryString.

0916dhkim · 2022-12-25T20:48:46Z

packages/docusaurus-theme-common/src/contexts/tabGroupChoice.tsx

+  // Sync storage, query params, and selected state.
+  useEffect(() => {
+    const queryParamValue =
+      searchKey && context.tabGroupChoicesInQueryParams[searchKey];
+    const storageValue = groupId && context.tabGroupChoicesInStorage[groupId];
+    const valueToSync = queryParamValue ?? storageValue;
+    const isValid =
+      !!valueToSync && values.some(({value}) => value === valueToSync);
+    if (isValid && valueToSync !== selectedValue) {
+      setSelectedValue(valueToSync);
+    }
+  }, [


Essence of this PR: Detect changes in local storage or query params, then update selectedValue accordingly.

slorber · 2022-12-27T18:38:45Z

Thanks

Will review this more deeply later, but in the meantime this PR breaks the querystring feature.

Opening this in a new session (clean/empty localstorage) should focus the iOS tab, but it doesn't anymore:
https://deploy-preview-8483--docusaurus-2.netlify.app/docs/markdown-features/tabs/?current-os=ios#query-string

slorber · 2022-12-28T11:53:13Z

Thanks for trying, but I'm going to open a different PR with a totally different way to handle things (useSyncExternalStore)

The current legacy way is not ideal and I'd like to get rid of this provider and react state synchronisation. As we can see, this leads to subtle bugs and is overly complex

0916dhkim · 2022-12-28T14:02:41Z

@slorber No problem! I was concerned about adding more to the context too, and now excited to see a better implementation 😄

slorber · 2022-12-28T18:14:32Z

Thanks

Closing this PR in favor of #8486

Can you help review and let me know if you see any issue?

Fix tab group sync

c44e113

0916dhkim requested review from slorber, lex111 and Josh-Cena as code owners December 25, 2022 20:37

facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 25, 2022

0916dhkim commented Dec 25, 2022

View reviewed changes

slorber mentioned this pull request Dec 28, 2022

fix(theme): refactor Tabs, make groupId + queryString work fine together #8486

Merged

slorber closed this Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix tab group sync #8483

Fix tab group sync #8483

0916dhkim commented Dec 25, 2022 •

edited by slorber

Loading

netlify bot commented Dec 25, 2022

github-actions bot commented Dec 25, 2022

0916dhkim left a comment •

edited

Loading

0916dhkim Dec 25, 2022

0916dhkim Dec 25, 2022

0916dhkim Dec 25, 2022

0916dhkim Dec 25, 2022

0916dhkim Dec 25, 2022

slorber commented Dec 27, 2022

slorber commented Dec 28, 2022

0916dhkim commented Dec 28, 2022

slorber commented Dec 28, 2022

		blockElementScrollPositionUntilNextRender(newTab);
		setSelectedValue(newTabValue);

Fix tab group sync #8483

Fix tab group sync #8483

Conversation

0916dhkim commented Dec 25, 2022 • edited by slorber Loading

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

netlify bot commented Dec 25, 2022

✅ [V2]

github-actions bot commented Dec 25, 2022

⚡️ Lighthouse report for the deploy preview of this PR

0916dhkim left a comment • edited Loading

Choose a reason for hiding this comment

Changes Summary

New structure

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

slorber commented Dec 27, 2022

slorber commented Dec 28, 2022

0916dhkim commented Dec 28, 2022

slorber commented Dec 28, 2022

0916dhkim commented Dec 25, 2022 •

edited by slorber

Loading

0916dhkim left a comment •

edited

Loading