Fix tab group sync by 0916dhkim · Pull Request #8483 · facebook/docusaurus

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

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

      : 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 Josh-Cena, lex111 and slorber 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

		blockElementScrollPositionUntilNextRender(newTab);
		setSelectedValue(newTabValue);

Uh oh!

Comments

Conversation

0916dhkim commented Dec 25, 2022 • edited by slorber Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-flight checklist

Motivation

Test Plan

Test links

Related issues/PRs

Uh oh!

netlify bot commented Dec 25, 2022

✅ [V2]

Uh oh!

github-actions bot commented Dec 25, 2022

⚡️ Lighthouse report for the deploy preview of this PR

Uh oh!

0916dhkim left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Changes Summary

New structure

Uh oh!

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

0916dhkim Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

slorber commented Dec 27, 2022

Uh oh!

slorber commented Dec 28, 2022

Uh oh!

0916dhkim commented Dec 28, 2022

Uh oh!

slorber commented Dec 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

0916dhkim commented Dec 25, 2022 •

edited by slorber

Loading

0916dhkim left a comment •

edited

Loading