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

Tabs - Fix issues for query-params-based status, nested tabs, variable tabs widths #1688

Merged
merged 19 commits into from Oct 11, 2023

Conversation

didoo
Copy link
Contributor

@didoo didoo commented Oct 3, 2023

📌 Summary

This is a "reworking" #1654 to understand if there was a more idiomatic (to HDS code) approach to solve the issue of tab selection and persistence using query params, initially proposed by @MiniHeyd in #1654.

In the process, I have also tried to tackle other two known tabs-related issues: the fact that when there are nested tabs, the indicator for the tabs inside hidden panels doesn't get initialized correctly, and the fact that when the content of the tab button (in particular the badge) changes, the indicator doesn't update its width accordingly.

The intent of this PR is to drive the conversation with @MiniHeyd and see how we can have a synthesis of the two PRs in a single one. Once approved the approach, the PR will be opened to the HDS engineering team for full review.

Notice: the documentation update will be done in #1702 and then merged here.

🛠️ Detailed description

In this PR I have:

  • refactored logic for Tabs component + Tab/Panel sub-components to support more complex use cases
  • fixed an issue with scrollIntoView scrolling the page on page load
  • introduced a way to notify nested tabs of updates on visibility and use it to fix issue with indicator not initialized for (hidden) nested tabs [not an elegant solution, but it works]
  • improved the handling of out-of-bound this.selectedTabIndex
  • added some more complex examples of tabs in showcase
  • increased test coverage for Tabs adding more detailed tests and complex use cases

👉 👉 👉 Preview: https://hds-showcase-git-selected-tab-condition-proposal-hashicorp.vercel.app/components/tabs (in particular see "Examples of tabs implementation")

🔗 External links

Jira tickets:


👀 Component checklist

  • Percy was checked for any visual regression
  • A11y tests have been run locally (yarn test:a11y --filter="COMPONENT-NAME")
  • A changelog entry was added via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-website ✅ Ready (Inspect) Visit Preview Oct 11, 2023 6:46am
1 Ignored Deployment
Name Status Preview Updated (UTC)
hds-showcase ⬜️ Ignored (Inspect) Visit Preview Oct 11, 2023 6:46am

@didoo
Copy link
Contributor Author

didoo commented Oct 5, 2023

@MiniHeyd as discussed today, the PR is ready for an initial review (I still have to update the documentation, but I'll do it in a separate follow-up PR tomorrow)

@hashicorp/hds-engineering it would be great if you could start to look at this PR, is pretty meaty and it would be better to get it merged in main before the 3.0 release so we don't block the TFC consumers in their Tabs adoption PRs (already opened).

(There are a few tests failing. I suspected they would have been brittle/flaky, I'll look into them asap)

@didoo didoo marked this pull request as ready for review October 5, 2023 20:51
@didoo didoo changed the title [WIP] Tabs - Fix issues for query-params-based status, nested tabs, variable tabs widths Tabs - Fix issues for query-params-based status, nested tabs, variable tabs widths Oct 6, 2023
@didoo didoo force-pushed the selected-tab-condition-proposal branch from 0171d1b to 826e54a Compare October 9, 2023 09:54
@didoo didoo force-pushed the selected-tab-condition-proposal branch from 826e54a to c5932b1 Compare October 9, 2023 11:52
didoo and others added 3 commits October 9, 2023 17:54
Co-authored-by: Steve Heydweiller <steve@highdwellercreative.com>
… it to fix issue with indicator not initialized for (hidden) nested tabs

not elegant solution, but it works
Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

I'm not sure what's causing the test failures, but after that's resolved, I have no objections to this PR after today's group review.

@didoo didoo force-pushed the selected-tab-condition-proposal branch from 60e1ad8 to c842b6e Compare October 10, 2023 10:42
@didoo
Copy link
Contributor Author

didoo commented Oct 10, 2023

@alex-ju this is ready for a second pass.

@MiniHeyd see my commit 8fd10b9 (added after your review)

Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Re-reviewed this one – had a more in-depth look at the functional aspects and checked the updated tests – and it's good to be merged. You'll likely want to get the documentation in first (which may require a rebase on this judging by the current diff no rebase required).

didoo and others added 2 commits October 11, 2023 07:42
Co-authored-by: Alex <alex-ju@users.noreply.github.com>
Co-authored-by: Jory Tindall <jory.tindall@hashicorp.com>
…documentation

`Tabs` - Update documentation
@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Oct 11, 2023
@didoo didoo merged commit 36d87a7 into main Oct 11, 2023
15 checks passed
@didoo didoo deleted the selected-tab-condition-proposal branch October 11, 2023 06:57
@hashibot-hds hashibot-hds mentioned this pull request Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-website Content updates to the documentation website packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants