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 header as persistence key #1602

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

chalin
Copy link
Collaborator

@chalin chalin commented Jul 4, 2023

This PR is a minimally intrusive/disruptive change to the tabpane shortcode, it:

  • Adds support for persisting tab headers

Headers are used as persistence key when tabpane.persistLang is absent and tabpane.lang is set. (The latter implies a base shared lang for all of the tabpane's tab -- which individual tabs can override.)

Persisting header keys is useful when a page contains multiple tabpanes sharing the same headers, regardless of the tab language used: e.g., see Collector > Getting Started for a page that could benefit from this. As an example of the impact of this change: it allows us to drop all 24 occurrences of persistLang=false from the OTel website.


Note: this PR has two commits. The two commits combined, provide minimal changes to tabpane. The first commit actually generalizes the values supported by tabpane.persistLang to be "lang", "header", or "false". If you prefer such a generalization from the start @deining, then I can drop the second commit (which is a partial revert).

Eventually, we might want to replace tabpane.persist supporting the values "lang", "header", or "false".

Depending on your feedback @deining, I'll add appropriate comments to the changelog in this or a followup PR.


This PR has no impact (other than whitespace cleanup) on the generated user guide:

$ npm run test
...
$ (cd userguide/public && git diff -bw)     
       0

(The same was true of the OTel website before I removed the persistLang=false attributes.)

@chalin chalin added the enhancement New feature or request label Jul 4, 2023
@chalin chalin requested review from LisaFC, deining and geriom July 4, 2023 21:04
@chalin chalin force-pushed the chalin-im-otel-tabpane+2023-07-04 branch from 943b49c to 0dcc6dc Compare July 4, 2023 21:05
@chalin
Copy link
Collaborator Author

chalin commented Jul 4, 2023

/cc @svrnm @cartermp

@chalin chalin changed the title tabpane: support persisting header as persistence key tabpane: support header as persistence key Jul 4, 2023
layouts/shortcodes/tabpane.html Outdated Show resolved Hide resolved
layouts/shortcodes/tabpane.html Outdated Show resolved Hide resolved
layouts/shortcodes/tabpane.html Show resolved Hide resolved
@deining
Copy link
Collaborator

deining commented Jul 5, 2023

This PR is a minimally intrusive/disruptive change to the tabpane shortcode, it:

In general I like the idea and consider this an improvement. I will approve this once all related topics are settled.

Note: this PR has two commits. The two commits combined, provide minimal changes to tabpane. The first commit actually generalizes the values supported by tabpane.persistLang to be "lang", "header", or "false". If you prefer such a generalization from the start @deining, then I can drop the second commit (which is a partial revert).

Yes, I would prefer to start from scratch here and allow values lang, header, or ´false` from start on.

Reading the documentation in the user guide I realize that more and more features were added to the shortcode by time, but we still have a textual description off all the available parameters only. I would prefer if we presented the available parameters inside a table (as we do for other shortcodes already). @chalin: are you willing to take that and come up with a initial draft here?

@chalin
Copy link
Collaborator Author

chalin commented Jul 5, 2023

Thanks for the feedback @deining.

Yes, I would prefer to start from scratch here and allow values lang, header, or ´false` from start on.

Good, that was my initial inclination and preference, but had a doubt as to whether you might want this feature added with minimal disruption or not. I'll pull out the second commit so that we can start from scratch, and make some adjustments.

@chalin chalin force-pushed the chalin-im-otel-tabpane+2023-07-04 branch from 0dcc6dc to 80a9be0 Compare July 5, 2023 11:02
@chalin
Copy link
Collaborator Author

chalin commented Jul 5, 2023

I've updated the PR so that it:

  • Keeps persistLang but warns that it is deprecated
  • Adds tabpane.persist as a parameter with supported values of header, lang, and none
    Note: I've opted for none rather than false, which makes more sense for a multivalued parameter IMHO

Regarding doc updates: yes the docs should be updated, but I'd like to land this PR first, in particular because I have some followup enhancements to suggest, and would like to approach this incrementally.

As you might have noticed, I added a warning to the README stating that main is "under development" and potentially unstable -- advising users to stick to official releases. So I think that an incremental approach is ok in that case.

Similar remark applies to renaming the persistLang() function. I'll handle that in a followup PR in conjunction with other changes.

PTAL

@deining
Copy link
Collaborator

deining commented Jul 5, 2023

I've updated the PR so that it:

* Keeps `persistLang` but warns that it is deprecated

Great!

* Adds `tabpane.persist` as a parameter with supported values of `header`, `lang`, and `none`
  **Note**: I've opted for `none` rather than `false`, which makes more sense for a multivalued parameter IMHO

Yes, I agree that none is more appropriate.

Regarding doc updates: yes the docs should be updated, but I'd like to land this PR first, in particular because I have some followup enhancements to suggest, and would like to approach this incrementally.

I see.

As you might have noticed, I added a warning to the README stating that main is "under development" and potentially unstable -- advising users to stick to official releases. So I think that an incremental approach is ok in that case.

Similar remark applies to renaming the persistLang() function. I'll handle that in a followup PR in conjunction with other changes.

Looking forward seeing more improvements coming in!

PTAL

LGTM now.

@chalin
Copy link
Collaborator Author

chalin commented Jul 5, 2023

LGTM now.

Great. Merging based on your LGTM.

@chalin chalin merged commit 37fb8ee into google:main Jul 5, 2023
5 checks passed
@chalin chalin deleted the chalin-im-otel-tabpane+2023-07-04 branch July 5, 2023 13:24
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.

2 participants