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

Tabpane: support persistence of all active tabs #1611

Merged
merged 8 commits into from
Jul 13, 2023

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Jul 6, 2023

  • Updates option value to disable persistence: now use persist=disabled (instead of off)
  • Enhances tabpane to support the persistence of the active tab selection for all tabpanes (excluding those with persist=disabled), rather than just the last activated tab
  • Restores all persisted activated tabs on page load
  • Persists in localStorage a count of the tab-activated events
  • Persists each activated tab in localStorage using the tab's persistence key. The value associated with the key is the tab-activated-event count plus one (also saved to localStorage). This supports typical use cases, in addition to the one explained below.
  • Drops button.onclick in favor of click event handlers.

Handling of non-uniform shared tabs

Assume that a page contains three tabpanes with the following distribution of tabs A, B, C, D -- where these letters represent the tab-persistence keys:

  1. A, B
  2. B, D
  3. A, B, C

If you activate tab B from any tabpane, it will become active in all three tabs and the selection will be persisted. If you then select C, then tab C will become active in tabpane 3, but tab panes 1 and 2 will remain active at B -- and these settings are persisted. If you then select A, then: tabpanes 1 and 3 become active at A, but tabpane 2 remains unchanged.

IMHO, this applies the principle of least surprise.

Preview and tests

Other

  • FYI, I've successfully tested this on the OTel website.
  • Updates to the changelog and docs will be made in a followup PR.

@chalin chalin added the enhancement New feature or request label Jul 6, 2023
@chalin chalin requested review from LisaFC, deining and geriom July 6, 2023 17:00
Copy link
Collaborator

@deining deining left a comment

Choose a reason for hiding this comment

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

Nice improvement! Thanks for your hard work!
LGTM, looking forward seeing this merged!
Once we manage to eventually review and incorporate long-standing PR #1202 and #1227, docsy theme will offer a really great comfort-solution for displaying ad handling tabs!

@chalin chalin force-pushed the chalin-im-multi-tab-persist-2023-07-06 branch from 28c9dee to 71fcf43 Compare July 13, 2023 16:25
@chalin chalin changed the title tabpane: support persistence of all active tabs Tabpane: support persistence of all active tabs Jul 13, 2023
@chalin chalin force-pushed the chalin-im-multi-tab-persist-2023-07-06 branch from 71fcf43 to efffaf0 Compare July 13, 2023 16:42
@LisaFC
Copy link
Collaborator

LisaFC commented Jul 13, 2023

This looks really good!

@chalin
Copy link
Collaborator Author

chalin commented Jul 13, 2023

Thanks for the reviews. As I mention in the associated issue, I'll update the tab-activation algorithm soonish, but this first version should be good enough for most sites. (I'll be fielding it soon too.)

@chalin chalin merged commit b53f5f1 into google:main Jul 13, 2023
6 checks passed
@chalin chalin deleted the chalin-im-multi-tab-persist-2023-07-06 branch July 13, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants