feat(v2): support syncing tab choices #2366

SamChou19815 · 2020-03-06T20:40:41Z

Motivation

Implement feature requested in #2349, but in a more general way.

The linked issue wants auto-syncing tabs for programming languages. However, I think it can be implemented in a more general way. For example, we might also use the tabs to separate OS-specific instructions. Therefore, instead of providing hooks like useProgrammingLanguages, I decide to provide a more general hook called useTabGroupChoiceContext.

No matter whether we decide to limit the feature to programming languages or general tabs, we need to worry about conflicts. For example, inside the same project, the docs might have both tabs for JavaScript vs TypeScript, and tabs for NPM vs Yarn. We don't want these two different tabs to interfere with each other in surprising ways.

To prevent unwanted interference, I decide to separate these choices by keys. Users must explicit provide the same groupId to convey to docusaurus that those tabs are related and their choices should be synced. To preserve backward compatibility, groupId is an optional prop of Tab. Not passing the prop means that the user opts out of tab choice syncing.

The choice is stored persistently in localStorage, just like the choice of theme.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

I added more examples of using tabs with groupId in website/docs/markdown-features.mdx to test the new feature.

(Some examples are silly, and I'm open to better examples.)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

docusaurus-bot · 2020-03-06T20:43:28Z

Deploy preview for docusaurus-2 ready!

Built with commit c0c7a32

https://deploy-preview-2366--docusaurus-2.netlify.com

lex111 · 2020-03-06T20:55:04Z

website/versioned_docs/version-2.0.0-alpha.43/markdown-features.mdx

 You may want to implement your own `<MultiLanguageCode />` abstraction if you find the above approach too verbose. We might just implement one in future for convenience.

+### Syncing Tab Choices


Document the new feature in website/docs. Please revert this change in versioned doc.

BTW do not use capitalization in heading.

lex111 · 2020-03-06T21:12:38Z

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

  const [selectedValue, setSelectedValue] = useState(defaultValue);
+
+  if (typeof groupId !== 'undefined') {


It seems to me that we can remove the groupId prop (this is somewhat redundant) in favor of value prop in TabItem component to identify each tab? Yes, this way we synchronize tabs by default and do not give users prevent this behavior. (Although I do not think that is a big problem).

lex111 · 2020-03-06T21:22:22Z

packages/docusaurus-theme-classic/src/theme/hooks/useTabGroupChoice.js

+  const setChoiceSyncWithLocalStorage = useCallback(
+    newChoices => {
+      try {
+        localStorage.setItem('tab-group', JSON.stringify(newChoices));


First, put storage value in separate const outside the hook-level. Second, add a namespace to that, for example docusaurus.tab-group.

put storage value in separate const outside the hook-level.

🆙 @SamChou19815

@lex111 Do you mean moving the literal'docusaurus.tab-choice' outside of the hooks?

Something like this (come up with a better name for const):

const STORAGE_KEY = ... const setChoiceSyncWithLocalStorage = ...

lex111 · 2020-03-06T21:25:29Z

packages/docusaurus-theme-classic/src/theme/TabGroupChoiceContext.js

+
+import React from 'react';
+
+const TabGroupChoiceContext = React.createContext({


Maybe use this approach?

import React, { createContext } from 'react'; const TabGroupChoiceContext = createContext(...)

SamChou19815 · 2020-03-06T22:39:25Z

Updated the PR is store only one choice

lex111 · 2020-03-06T22:41:50Z

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

  const [selectedValue, setSelectedValue] = useState(defaultValue);
+
+  console.log({values, tabGroupChoice});


Logging should be removed

Oops. I will remove it.

lex111 · 2020-03-07T10:16:13Z

packages/docusaurus-theme-classic/src/theme/hooks/useTabGroupChoice.js

+  const setChoiceSyncWithLocalStorage = useCallback(
+    newChoice => {
+      try {
+        localStorage.setItem(TAB_CHOICE_STORAGE_KEY, newChoice);


tab-choice is must be an array in order to maintain sync for several different tabs.

See https://deploy-preview-2366--docusaurus-2.netlify.com/docs/markdown-features/ for example.

I think a plain array can work if all the tab choices happen to be disjoint. However, if we have something like ['win', 'mac'] and ['win', 'unix'] and we see ['win'] in the array, there is an ambiguity about which win it is referring to.

Should we make it an object keyed by serialized values array?

Not sure if this is necessary, so regular array should work. We assume that users will use the same tab names for similar tabs (Yarn VS npm eg) throughout the website.

yangshun

This is so awesome! I really like that you made it generic! Thank you @SamChou19815!

What do you think about adding a new prop to <Tabs /> called groupKey? The presence of this prop means that that all the Tabs instance of this groupKey wants to be grouped and updated together. With such a groupKey, a few problems with the current approach is solved:

The current choices data structure doesn't know which Tabs it originally belongs to and there might be collisions with new Tabs of different purpose but have the same value (chances are slim but you never know).
Allows saving each individual choice in its own localStorage key instead of stringifying the entire choices object (e.g. docusaurus.tabs.operating_system, docusaurus.tabs.package_manager). Also eliminates the possibility of a corrupted value for one groupKey affecting the other groupKeys.

Lemme know your thoughts!

EDIT: Just read the comments above and realized my suggested approach was exactly what you did. @lex111 I believe using a groupId/groupKey is better for engineering stability and maintainability even if it requires more work from the user. Docusaurus tries to be explicit and avoid magic behind the scenes. Apologies for the conflicting opinions!

yangshun · 2020-03-11T14:50:15Z

packages/docusaurus-theme-classic/src/theme/hooks/useTabGroupChoice.js

+  useEffect(() => {
+    try {
+      const localStorageChoice = localStorage.getItem(TAB_CHOICE_STORAGE_KEY);
+      if (localStorageChoice !== null) {


We can do != null.

yangshun · 2020-03-11T15:07:45Z

packages/docusaurus-theme-classic/src/theme/hooks/useTabGroupChoice.js

+
+import {useState, useCallback, useEffect} from 'react';
+
+const TAB_CHOICE_STORAGE_KEY = 'docusaurus.tab-choice';


docusaurus.tabs would be sufficient.

yangshun

Thanks so so so much @SamChou19815! I made some minor edits to the docs to add explanations and make it a little clearer. Awesome work! 🎉

SamChou19815 requested review from lex111, wgao19 and yangshun as code owners March 6, 2020 20:40

facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 6, 2020

lex111 reviewed Mar 6, 2020

View reviewed changes

lex111 added the pr: new feature This PR adds a new API or behavior. label Mar 6, 2020

lex111 reviewed Mar 6, 2020

View reviewed changes

lex111 reviewed Mar 7, 2020

View reviewed changes

SamChou19815 requested a review from lex111 March 8, 2020 23:37

yangshun suggested changes Mar 11, 2020

View reviewed changes

yangshun reviewed Mar 11, 2020

View reviewed changes

SamChou19815 added 8 commits March 14, 2020 01:24

feat(v2): Support syncing tab choices

36e783f

Move docs changes to website/docs

c81ab32

Do not import entire React in TabGroupChoiceContext

80cc275

Store only one tab choice according to discussion in PR

ef2479c

Remove leftover logging code during debugging

d1cbcf8

Put storage value in separate const outside the hook-level

9e79914

Use an array to keep track of different tab groups

8d5d6b3

Revert back to using groupId

8931dd3

SamChou19815 requested a review from yangshun March 14, 2020 06:06

yangshun changed the title ~~feat(v2): Support syncing tab choices~~ feat(v2): support syncing tab choices Mar 15, 2020

yangshun added 2 commits March 15, 2020 13:36

Update markdown-features.mdx

e79f03e

Update markdown-features.mdx

c0c7a32

yangshun approved these changes Mar 15, 2020

View reviewed changes

yangshun merged commit 55a50d3 into facebook:master Mar 15, 2020

yangshun mentioned this pull request Mar 15, 2020

Changing the language for one code tab should automatically change all code tabs to that language. #2349

Closed

SamChou19815 deleted the use-tab-group-choices-hook branch March 15, 2020 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(v2): support syncing tab choices #2366

feat(v2): support syncing tab choices #2366

SamChou19815 commented Mar 6, 2020 •

edited

Loading

docusaurus-bot commented Mar 6, 2020 •

edited

Loading

lex111 Mar 6, 2020

lex111 Mar 6, 2020

lex111 Mar 6, 2020

lex111 Mar 6, 2020

SamChou19815 Mar 6, 2020

lex111 Mar 6, 2020

lex111 Mar 6, 2020

SamChou19815 commented Mar 6, 2020

lex111 Mar 6, 2020

SamChou19815 Mar 6, 2020

lex111 Mar 7, 2020

SamChou19815 Mar 7, 2020

lex111 Mar 7, 2020

yangshun left a comment •

edited

Loading

yangshun Mar 11, 2020

yangshun Mar 11, 2020

yangshun left a comment

		You may want to implement your own `<MultiLanguageCode />` abstraction if you find the above approach too verbose. We might just implement one in future for convenience.

		### Syncing Tab Choices

		const [selectedValue, setSelectedValue] = useState(defaultValue);

		if (typeof groupId !== 'undefined') {


		import React from 'react';

		const TabGroupChoiceContext = React.createContext({

		const [selectedValue, setSelectedValue] = useState(defaultValue);

		console.log({values, tabGroupChoice});


		import {useState, useCallback, useEffect} from 'react';

		const TAB_CHOICE_STORAGE_KEY = 'docusaurus.tab-choice';

feat(v2): support syncing tab choices #2366

feat(v2): support syncing tab choices #2366

Conversation

SamChou19815 commented Mar 6, 2020 • edited Loading

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

Related PRs

docusaurus-bot commented Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamChou19815 commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangshun left a comment

Choose a reason for hiding this comment

SamChou19815 commented Mar 6, 2020 •

edited

Loading

docusaurus-bot commented Mar 6, 2020 •

edited

Loading

yangshun left a comment •

edited

Loading