Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 20 commits into from Dec 9, 2022

Conversation

mturoci
Copy link
Contributor

@mturoci mturoci commented Oct 18, 2022

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:
  • 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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 18, 2022
@netlify
Copy link

netlify bot commented Oct 18, 2022

[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...

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
Copy link

github-actions bot commented Oct 18, 2022

⚡️ 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

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines -72 to +88
// 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)
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain the intent here? Not sure to understand

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved
packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved
packages/docusaurus-theme-classic/src/theme/Tabs/index.tsx Outdated Show resolved Hide resolved
@slorber
Copy link
Collaborator

slorber commented Oct 20, 2022

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

CleanShot 2022-10-20 at 19 47 23

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.

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 20, 2022
@mturoci
Copy link
Contributor Author

mturoci commented Oct 24, 2022

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!

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

@slorber slorber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks better but not yet good enough 🤪

@slorber
Copy link
Collaborator

slorber commented Oct 26, 2022

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
Copy link
Contributor Author

mturoci commented Oct 30, 2022

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
Copy link

jnikhila commented Dec 5, 2022

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

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

slorber commented Dec 9, 2022

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
Copy link
Collaborator

slorber commented Dec 9, 2022

Looks like it works as I want it to 👍

Will normally be in upcoming v2.3

@slorber slorber merged commit 5c09dbf into facebook:main Dec 9, 2022
@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Dec 9, 2022
@0916dhkim 0916dhkim mentioned this pull request Dec 25, 2022
3 tasks
@zeluspudding
Copy link

zeluspudding commented Jan 12, 2023

Any word on when 2.3 becomes publicly available?

@slorber
Copy link
Collaborator

slorber commented Jan 18, 2023

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

slorber pushed a commit that referenced this pull request Jan 26, 2023
Co-authored-by: sebastienlorber <lorber.sebastien@gmail.com>
Closes #7008
@slorber 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open respective tab item with URL query string
5 participants