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

feat(components): add post-tabs component #1181

Merged
merged 53 commits into from
Aug 24, 2023
Merged

Conversation

alizedebray
Copy link
Contributor

No description provided.

@alizedebray alizedebray linked an issue Feb 24, 2023 that may be closed by this pull request
11 tasks
@changeset-bot
Copy link

changeset-bot bot commented Feb 24, 2023

🦋 Changeset detected

Latest commit: f024746

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@swisspost/design-system-documentation Minor
@swisspost/design-system-components Minor
@swisspost/design-system-styles Patch
@swisspost/design-system-components-react Patch
@swisspost/design-system-documentation-v6 Patch
@swisspost/design-system-components-angular Patch
@swisspost/design-system-demo Patch
@swisspost/internet-header Patch
@swisspost/design-system-intranet-header Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@swisspost-bot
Copy link
Contributor

swisspost-bot commented Feb 24, 2023

Preview environment ready: https://preview-1181--swisspost-web-frontend.netlify.app
Preview environment ready: https://preview-1181--swisspost-design-system-next.netlify.app
Preview environment ready: https://preview-1181--swisspost-design-system-next-v7.netlify.app

@alizedebray alizedebray changed the title Feat/229 component tabs feat(components): add post-tabs component Feb 24, 2023
@alizedebray alizedebray changed the base branch from main to chore/refactor-utils-from-components-package February 24, 2023 16:14
Base automatically changed from chore/refactor-utils-from-components-package to main March 6, 2023 10:10
# Conflicts:
#	packages/components/src/utils/index.ts
#	packages/components/src/utils/property-checkers/index.ts
#	pnpm-lock.yaml
@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@gfellerph
Copy link
Member

gfellerph commented Jul 27, 2023

A lot of slot checks are happening. Can this be simplified?

Some thoughts on the API design

Instead of checking if things are assigned to the correct slot, we could also auto-assign to the correct slot (by adding slot="heading" to post-tab-header child-components) because we know which element has to go where.

The slot design could be simplified by defining a default slot for panels, additional content will just always be shown in the context of the tabs (maybe authors even want that, e.g. inserting static content between headers and panels).

In my opinion, it would be easier if authors only had to deal with the post-tabs and not the sub-components when listening to changes and programmatically activating/deactivating panels. This would elevate post-tabs to be the controller of the leaf-components post-tab-header and post-tab-panel.

At the moment, tabs get their index themselves based on the order in markup. This might lead to complex to fix issues if there are dynamic tabs that change during runtime and get created in async order. How about we require a prop to link header to panel, e.g. id and for? Then a simple querySelector can always select the correct panel and we have less internal state to maintain. This way it gets easy to set the id on the header and the aria-labelledby on the panel. This will also ensure that authors choose a unique id for the panel and link it accordingly from the header (no need for tabsGroup anymore). It puts a little more work on the author, but will help to make the implementation rock solid.

I don't think it's necessary to watch the active prop on the header as it should just define an initial state. Later changes should be triggered by the user or the show method on post-tabs.

We could define the interfaces like so:

post-tabs
Props: none
Methods: show(name or index) [no hide necessary, one tab has to be active at all times and authors should be explicit on what tab should be shown]
Events: tabChange(name)

post-tab-header
Props: active (first occurrence is activated), for (links to the panel)
Methods: none
Events: none

post-tab-panel
Props: id (links to the header)
Methods: none
Events: none

If we move methods to the controller, it's easier to sync the active state.

@gfellerph gfellerph self-assigned this Jul 27, 2023
@alizedebray
Copy link
Contributor Author

On Firefox (at least), after multiple clicks, the content of the tab disappears:

Good point, I prevented tab switch when clicking an active tab.

Copy link
Member

@gfellerph gfellerph left a comment

Choose a reason for hiding this comment

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

We can also open tickets for fixes that we want to deliver later.

@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@gfellerph gfellerph left a comment

Choose a reason for hiding this comment

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

Can't believe it, but it finally happens.

@alizedebray alizedebray merged commit b41db4b into main Aug 24, 2023
10 of 11 checks passed
@alizedebray alizedebray deleted the feat/229-component-tabs branch August 24, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component: Tabs
5 participants