feat(theme-classic): store selected tab in query string. #8225

mturoci · 2022-10-18T17:05:52Z

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.

Implementation description

Selection stored according to this comment - serialized JSON with group ids as keys and __noGroup__ constant key for tabs that do not belong to any group. Downsides:
- Ugly links - http://localhost:3000/docs/markdown-features/tabs?tabs=%7B%22__noGroup__%22%3A%22banana%22%2C%22operating-systems%22%3A%22win%22%7D
- In case of multiple tabs that do not belong to any group and have different tab item keys, this solution would only work for the last selected tab.
Selected key priority: search query > local storage > defaultValue prop > 1st tab with default specified > 1st tab.
@slorber can you please provide further explanation for this comment ? I identified the file that needs the changes as this, but it's not clear to me what is actually needed to be done. From my understanding, when you hydrate React components and build static HTML off it, you cannot know the search URL query params as they are only available during the actual request or am I missing something?

Let's use this PR as starting point for further discussion around UX/implementation.

Motivation

Allows for sharing tabs selection via URL - better UX.

Test Plan

Manual

Test links

Related issues/PRs

Closes #7008

netlify · 2022-10-18T17:32:45Z

✅ [V2]

Name	Link
🔨 Latest commit	`c2eca08`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/63935ba7088f13000883eb46
😎 Deploy Preview	https://deploy-preview-8225--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-10-18T17:33:20Z

⚡️ Lighthouse report for the deploy preview of this PR

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

slorber

Thanks

Not good enough to merge as is.

Sync between groups is now broken: https://deploy-preview-8225--docusaurus-2.netlify.app/docs/markdown-features/tabs/#syncing-tab-choices

I'd rather not serialize as JSON.

I'd also prefer if we opt-in for querystring serialization instead of making it a default

What I'd do:

provide queryString: string | boolean prop
false by default => no querystring
true => use groupId as querystring param
string => use value as querystring param

slorber · 2022-10-20T17:38:56Z

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

-  // When defaultValueProp is null, don't show a default tab
-  const defaultValue =
-    defaultValueProp === null
-      ? defaultValueProp
-      : defaultValueProp ??
-        children.find((child) => child.props.default)?.props.value ??
-        children[0]!.props.value;
-  if (defaultValue !== null && !values.some((a) => a.value === defaultValue)) {
+
+  // Warn user about passing incorrect defaultValue as prop.
+  if (
+    defaultValueProp !== null &&
+    defaultValueProp !== undefined &&
+    !values.some((a) => a.value === defaultValueProp)
+  ) {


can you explain the intent here? Not sure to understand

Very similar to the prev check. However, this new one only throws in case of user-error. E.g. available tab keys are ke1, key2, key3, but user supplies key4. The goal is to notify user it's their fault that the tabs are not working properly.

The prev implementation would throw even if the error was on the Docusaurus (implementation) side as it checked the derived value instead of user supplied one.

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

slorber · 2022-10-20T17:49:39Z

For the hydration problem, I don't know exactly how we can solve it, but if would be really preferably if we didn't have that "hydration tab change" when React loads:

https://deploy-preview-8225--docusaurus-2.netlify.app/docs/markdown-features/tabs?tabs=%7B%22__noGroup__%22%3A%22banana%22%2C%22operating-systems%22%3A%22win%22%7D

This can be particularly disturbing in case tabs have different heights. If an anchor is provided in addition to qs tabs, it can also mess-up with the scroll position.

Now this is likely not easy to solve so I won't block the PR if it can't be fixed, it can be handled later.

mturoci · 2022-10-24T14:24:30Z

preferably if we didn't have that "hydration tab change" when React loads

Yes, the desired outcome is clear. The implementation is the question here since docusaurus uses SSG, not SSR. Curious about the solution though.

@slorber I have added the desired react router APIs, but unfortunately these break unit tests as they are not renderable on the server. I wasn't able to find similar scenario (react router APIs within unit tests) throughout the codebase. Could you please give me some pointers? Thanks!

slorber

Thanks, looks better but not yet good enough 🤪

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

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

slorber · 2022-10-26T17:27:32Z

Yes, the desired outcome is clear. The implementation is the question here since docusaurus uses SSG, not SSR. Curious about the solution though.

Unfortunately that's exactly why it's not easy. There are tricks to work around this in SSG (see dark mode or ThemedImage). Don't worry, we'll eventually figure out later in another PR.

mturoci · 2022-10-30T18:13:16Z

looks better but not yet good enough 🤪

No worries, it's expected to be an iterative process so that we are on the same page as for the required implementation direction :)

I don't see how a function call is overkill here.

I see we are from different camps. Clean code is very subjective and although I don't agree with your opinions, I fully respect them and since you are the maintainer here, you have the upper hand ofc :D Refactored into more abstractions as requested. Let me know if that's enough or you would like me to split the custom hook into a separate file as well.

There are tricks to work around this in SSG (see dark mode or ThemedImage)

Interesting, so basically the solution here would be to render every possible value as default value (in case of tabs with 3 options, that would be 3 separate Tabs component instances) and then based on the query param (that we can get by running a render-blocking script prior to actual rendering) determine, which Tabs shall be hidden and which shall be shown, is that right?

jnikhila · 2022-12-05T07:44:04Z

Hey! We are keen to use this feature for our documentation. Could you please confirm the release date?

…ges from <Tabs queryString>

slorber · 2022-12-09T15:15:55Z

Did some refactoring, fixes and docs improvements => should be ready to review.

Some things to review more carefully:

clicking a tab should not lead to scroll position changes (it did due to core scrollAfterNavigation())
how groupId and queryString work together => it makes sense to me the url qs choice overrides the localStorage choice

Please test the preview and let me know if you see anything weird

slorber · 2022-12-09T16:45:55Z

Looks like it works as I want it to 👍

Will normally be in upcoming v2.3

zeluspudding · 2023-01-12T16:19:53Z

Any word on when 2.3 becomes publicly available?

slorber · 2023-01-18T16:04:00Z

soon, I just come back from holiday and have a hundred things to check first

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com> Closes #7008

feat(theme-classic): store selected tab in query string.

c621e42

mturoci requested review from slorber and Josh-Cena as code owners October 18, 2022 17:05

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 18, 2022

test: fix failing tests

634c0f2

slorber requested changes Oct 20, 2022

View reviewed changes

slorber added the pr: new feature This PR adds a new API or behavior. label Oct 20, 2022

mturoci added 3 commits October 24, 2022 15:33

chore: Use react router instead of native window location.

52fa90b

fix: Make tabs sync work again.

04053c5

test: Temporarily skip tabs tests.

216119d

mturoci requested review from slorber and removed request for Josh-Cena October 24, 2022 14:24

fix: Make queryString param optional.

cabac96

slorber requested changes Oct 26, 2022

View reviewed changes

mturoci added 5 commits October 30, 2022 17:21

test: Remove tab unit test skip.

80e978d

feat: Querystring user props valdiation.

6e1fb03

chore: Remove unnecessary env var.

7eb1334

chore: Refactor.

66a3939

docs: Document the tab querystring behavior.

4213dc2

mturoci added 2 commits October 30, 2022 19:20

chore: Remove unnecessary and outdated comments.

60813d8

fix: Remove forgotten code for default value.

5e38a1f

slorber added 2 commits December 9, 2022 15:16

better querystring implementation refactor

f8eb751

better tab querystring doc

13aec5d

slorber added 4 commits December 9, 2022 15:45

ensure queryString wins over localStorage

3d2a36f

doc

0d48df6

// highlight-next-line

5264f83

fix scrollAfterNavigation logic to not mess-up with query-string chan…

1bd6561

…ges from <Tabs queryString>

slorber requested a review from lex111 as a code owner December 9, 2022 15:13

slorber approved these changes Dec 9, 2022

View reviewed changes

fix tab selected value restoration logic

c2eca08

slorber merged commit 5c09dbf into facebook:main Dec 9, 2022

slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Dec 9, 2022

Josh-Cena mentioned this pull request Dec 21, 2022

Tab sync using groupId not functioning #8473

Closed

7 tasks

0916dhkim mentioned this pull request Dec 25, 2022

Fix tab group sync #8483

Closed

3 tasks

slorber pushed a commit that referenced this pull request Jan 26, 2023

feat(theme-classic): store selected tab in query string. (#8225)

78d5a53

Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com> Closes #7008

slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Jan 27, 2023

jtoar mentioned this pull request Jan 29, 2023

fix(deps): update docusaurus monorepo to v2.3.0 redwoodjs/redwood#7475

Merged

1 task

slorber mentioned this pull request Feb 1, 2023

Table of Contents don't work using Tabs #5343

Closed

Dr-Electron mentioned this pull request Apr 23, 2023

Update docusaurus to 2.4.1 iotaledger/iota-wiki#937

Merged

4 tasks

Dr-Electron mentioned this pull request Jul 10, 2023

Make tabs linkable iotaledger/iota-wiki#1032

Closed

slorber mentioned this pull request Oct 19, 2023

chore: v3.0.0-rc.0 release #9418

Merged

dereknola mentioned this pull request Nov 17, 2023

Move Advanced OS preop into requirements k3s-io/docs#199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(theme-classic): store selected tab in query string. #8225

feat(theme-classic): store selected tab in query string. #8225

mturoci commented Oct 18, 2022 •

edited by slorber

Loading

netlify bot commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022 •

edited

Loading

slorber left a comment

slorber Oct 20, 2022

mturoci Oct 24, 2022

slorber commented Oct 20, 2022 •

edited

Loading

mturoci commented Oct 24, 2022

slorber left a comment

slorber commented Oct 26, 2022

mturoci commented Oct 30, 2022 •

edited

Loading

jnikhila commented Dec 5, 2022

slorber commented Dec 9, 2022

slorber commented Dec 9, 2022 •

edited

Loading

zeluspudding commented Jan 12, 2023 •

edited

Loading

slorber commented Jan 18, 2023

feat(theme-classic): store selected tab in query string. #8225

feat(theme-classic): store selected tab in query string. #8225

Conversation

mturoci commented Oct 18, 2022 • edited by slorber Loading

Pre-flight checklist

Implementation description

Motivation

Test Plan

Test links

Related issues/PRs

netlify bot commented Oct 18, 2022 • edited Loading

✅ [V2]

github-actions bot commented Oct 18, 2022 • edited Loading

⚡️ Lighthouse report for the deploy preview of this PR

slorber left a comment

Choose a reason for hiding this comment

slorber Oct 20, 2022

Choose a reason for hiding this comment

mturoci Oct 24, 2022

Choose a reason for hiding this comment

slorber commented Oct 20, 2022 • edited Loading

mturoci commented Oct 24, 2022

slorber left a comment

Choose a reason for hiding this comment

slorber commented Oct 26, 2022

mturoci commented Oct 30, 2022 • edited Loading

jnikhila commented Dec 5, 2022

slorber commented Dec 9, 2022

slorber commented Dec 9, 2022 • edited Loading

zeluspudding commented Jan 12, 2023 • edited Loading

slorber commented Jan 18, 2023

mturoci commented Oct 18, 2022 •

edited by slorber

Loading

netlify bot commented Oct 18, 2022 •

edited

Loading

github-actions bot commented Oct 18, 2022 •

edited

Loading

slorber commented Oct 20, 2022 •

edited

Loading

mturoci commented Oct 30, 2022 •

edited

Loading

slorber commented Dec 9, 2022 •

edited

Loading

zeluspudding commented Jan 12, 2023 •

edited

Loading