-
Notifications
You must be signed in to change notification settings - Fork 60
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
[ENH] Sticky sync tabs and URL param #104
Conversation
Thanks for submitting your first pull request! You are awesome! 🤗 |
0f4f48f
to
816517d
Compare
@chrisjsewell , please let me know if there's something I need to do before a review. Also, I'm completely open to suggestion on changes. |
Hey @mikemckiernan, sorry I wasn't able to look at this just yet, maybe @choldgraf could have a quick look? |
Yep, it'd be great if @choldgraf could peek and provide feedback. I appreciate everyone's time and help. Great project too! |
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.
This looks helpful, thanks - a few quick questions on the UX for this.
To include the tab as a URL parameter, add a setting like the following example in your `conf.py` file: | ||
|
||
```python | ||
sphinx_design_sync_tabs_url_param = "tab" |
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.
What's the benefit of making this configurable? My general preference is to keep our configuration surface area as small as possible unless somebody explicitly asks for extra functionality. E.g., just force the url param to be something like sd-last-tab
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.
The benefit of making it configurable is that the URL is customer-facing. The two common URL search param values that I've seen in the wild are code
and tab
. My thinking is that the tabs don't always show code
, so that felt wrong to hard code. If you prefer a single, hard-coded value, my preference is tab
and I can revise it so.
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.
my general inclination is always to start with the simplest-humanly-possible configuration/implementation, and only complexify once there's clear demand for it, in order to minimize our maintenance burden and the likelihood of deprecations, given that these projects all have pretty limited ongoing maintenance time.
So I'd suggest we ask this question: how important do you think it is to be able to configure the URL param? If >20% of users would want to configure it, let's add it here. If <20% would, then let's keep it simple and see if people request this in the future.
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.
After revising, I forgot one more reason for making the URL search param configurable. If the end user does not set it, then the URL is not set with a search param after clicking a tab (though stickiness still works because local storage is set). Originally, I thought this was a decent compromise for backward compatibility since the current release doesn't set a URL search param.
const storageKey = '&{storage_key}'; | ||
// The default value for paramKey is the string 'None'. That value indicates | ||
// not to set a URL search parameter. | ||
const paramKey = '&{param_key}'; |
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'm a bit confused about the UX if people have multiple groups of synced tabs throughout their site. In that case would it just store the state of the synced tab group that was last-clicked?
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.
That sounds correct--if one Sphinx project has a set of synchronized tabs with python
, java
, and c
and another set with lua
and shell
, or windows
and linux
for that matter, then whatever is clicked last is set in the storage key. We'd need add another option for the tab and store the data so folks could specify the python-java-c
sets differently from the windows-linux
sets.
I was thinking that the typical use is for a Sphinx project to use the tabs for programming language, platform, or user interface. If you feel that isn't featureful enough and want to add an option, LMK and I'll see what I can do.
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.
yep I think you're probably right, but we should just clarify this in the docs so that people know what to expect. My feeling is that for now we should just assume a single set of synced tabs, and expand the complexity only if people request it explicitly
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 typed more about the UX, though I might have poisoned the problem.
https://sphinx-design--104.org.readthedocs.build/en/104/tabs.html#save-tab-state-across-visits
Oh, and you can be the second person with first-hand user experience: https://sphinx-design--104.org.readthedocs.build/en/104/tabs.html?tab=rst
@choldgraf , thanks for the feedback. PLMK if there's anything else you'd like changed or that I can clarify. |
@choldgraf , @chrisjsewell , can I get another look at this PR? I'd like to get my org to bring in this change from upstream. Thanks and let me know if there's anything else to revise. |
@choldgraf this is still awaiting your feedback |
Add a Sphinx configuration option that is named `sphinx_design_sync_tabs_storage_key` so that users can set a project-specific local storage key rather than risk clobbering the value for `sphinx-design-last-tab` across different projects. Add support for an optional URL search param of like `?tab=blah` or `?code=blah` so that I can copy and paste a URL and the receiver sees the tab content that I see. By default, no param and you get to add "Oh, and click blah" in your message. If the URL has search param `?tab=blah`, set the local storage value to `blah`. If `blah` matches a synchronization key for a tab, show the tab. When someone clicks a tab, set the local storage value to the synchronization key value and also update the URL search params with `?tab=<value>` if the project configures a URL param.
1e1a002
to
c8c0fd6
Compare
@choldgraf , @chrisjsewell , any chance of another look? I'm running out of runway. Thanks very much. |
In our project we used to use |
Addresses #103.
Add a Sphinx configuration option that is named
sphinx_design_sync_tabs_storage_key
so that I can set a project-specific local storage key rather than risk clobbering the value forsphinx-design-last-tab
across different projects.Add support for a URL search param
?tab=blah
so that I can copy and paste a URL and the receiver sees the tab
content that I see. The URL search param is set in browser
local storage when you click a tab.
If you visit a URL with search param
?tab=blah
, set thelocal storage value to
blah
. Ifblah
matchesa synchronization key for a tab, show the tab.