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

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

Merged
merged 7 commits into from Dec 29, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Dec 28, 2022

Motivation

Fix #8473 (groupId/localStorage sync regression)

Much needed component refactor, make it easier to reason about and move most of the technical code to theme-common

Test Plan

Unit tests + preview

Test links

A good case to test: select "ios" on the 2nd/last tabs with a groupId (this will be persisted in localstorage)

(querystring wins over localstorage on purpose)

Related issues/PRs

Supersed #8483

@slorber slorber added the pr: bug fix This PR fixes a bug in a past release. label Dec 28, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 28, 2022
@slorber slorber mentioned this pull request Dec 28, 2022
3 tasks
@netlify
Copy link

netlify bot commented Dec 28, 2022

[V2]

Name Link
🔨 Latest commit 3acb760
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63ad7390a4299200094f3c6a
😎 Deploy Preview https://deploy-preview-8486--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 Dec 28, 2022

⚡️ Lighthouse report for the deploy preview of this PR

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

@slorber
Copy link
Collaborator Author

slorber commented Dec 28, 2022

If anyone want to review this asap please let me know otherwise I'd like to merge it and do a release tomorrow

@github-actions
Copy link

github-actions bot commented Dec 28, 2022

Size Change: +95 B (0%)

Total Size: 888 kB

Filename Size Change
website/build/assets/js/main.********.js 665 kB +95 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 70.8 kB
website/build/assets/css/styles.********.css 112 kB
website/build/index.html 40.9 kB

compressed-size-action

Copy link
Contributor

@0916dhkim 0916dhkim left a comment

Choose a reason for hiding this comment

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

Using useSyncExternalStore hook makes much more sense than using React context 👍

packages/docusaurus-theme-common/src/utils/storageUtils.ts Outdated Show resolved Hide resolved
packages/docusaurus-theme-common/src/utils/tabsUtils.tsx Outdated Show resolved Hide resolved
@slorber
Copy link
Collaborator Author

slorber commented Dec 29, 2022

LocalStorage sync now also works across windows 🥳

CleanShot.2022-12-29.at.12.00.20.mp4

@slorber slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Dec 29, 2022
@slorber slorber merged commit 9c860ce into main Dec 29, 2022
@slorber slorber deleted the slorber/fix-tabs-persistence branch December 29, 2022 11:41
@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: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab sync using groupId not functioning
3 participants