-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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 tab group sync #8483
Fix tab group sync #8483
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
updatesselectedValue
, local storage (ifgroupId
is given), and query params (ifqueryString
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.
const defaultValue = | ||
defaultValueProp !== undefined | ||
? defaultValueProp | ||
: children.find((child) => child.props.default)?.props.value ?? | ||
children[0]!.props.value; | ||
|
||
const [selectedValue, setSelectedValue] = useState(defaultValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manage selectedValue
state inside useTabGroupChoice
hook.
blockElementScrollPositionUntilNextRender(newTab); | ||
setSelectedValue(newTabValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/** 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}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove
ready
property. - Manage both local storage values & query param values in this context.
(groupId: string, newChoice: string) => { | ||
setChoices((oldChoices) => ({...oldChoices, [groupId]: newChoice})); | ||
setChoiceSyncWithLocalStorage(groupId, newChoice); | ||
const setTabGroupChoice = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setTabGroupChoice
pushes tab choice to local storage or query param depending on groupId
& queryString
.
// 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); | ||
} | ||
}, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Essence of this PR: Detect changes in local storage or query params, then update selectedValue
accordingly.
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: |
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 |
@slorber No problem! I was concerned about adding more to the context too, and now excited to see a better implementation 😄 |
Thanks Closing this PR in favor of #8486 Can you help review and let me know if you see any issue? |
Pre-flight checklist
Motivation
Fixing #8473
Test Plan
Test links
Deploy preview: https://deploy-preview-8483--docusaurus-2.netlify.app/
Related issues/PRs
fix #8473
#8225