Skip to content

fix(tabs)!: tabscontroller refactor #2699

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.

Sign up for GitHub

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

Jump to bottom
Merged
bennypowers merged 17 commits into from
Mar 6, 2024
Merged

fix(tabs)!: tabscontroller refactor #2699

bennypowers merged 17 commits into from
Mar 6, 2024

Conversation

@bennypowers
Copy link
Member

@bennypowers bennypowers commented Mar 3, 2024 •
edited
Loading

Closes #2695

What I did

Total separation between RTIC and tabs state. Disabled tabs are still able to be focused via RTIC, just that they can't change the Tab's active tab state.

Remove most of tabscontroller's logic in favour of context, rtic.

  • Rename TabsController > TabsAriaController. It still has some helper functions. WDYT, @zeroedin?
  • PfTabs(host) - context for active index, cascading stuff like variant, box etc
  • PfTab(host) - updates own template accd to context. to avoid breaking css selectors, maybe reflect those context props for now. at least investigate it before removing the attrs on the host.

Testing Instructions

  1. units, usability testing

@bennypowers
docs(select): update changeset
062a2cb
@bennypowers
fix(tabs)!: refactor controllers and tabs state
95ef241
@bennypowers
fix(tabs)!: refactor to simplify tabscontroller
cf5dd22 
let tabscontroller handle the aria stuff only, for the most part.
Total separation between RTIC and tabs state.
Disabled tabs are still able to be focused via RTIC,
just that they can't change the Tab's active tab state.
Use context instead of cascade
Use shadow classes instead of reflected attrs (but keep the attrs for now to avoid breaking changes)
@bennypowers bennypowers requested review from nikkimk and zeroedin March 3, 2024 13:01
@changeset-bot
Copy link

changeset-bot bot commented Mar 3, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 0d19f99

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

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

@github-actions github-actions bot added demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools labels Mar 3, 2024
@netlify
Copy link

netlify bot commented Mar 3, 2024 •
edited by github-actions bot
Loading

✅ Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit d9bcbb8
😎 Deploy Preview https://deploy-preview-2699--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the AT passed Automated testing has passed label Mar 3, 2024
zeroedin
zeroedin reviewed Mar 4, 2024
View reviewed changes
@zeroedin
Copy link
Contributor

zeroedin commented Mar 4, 2024 •
edited
Loading

Should we now say that if <pf-tab aria-disabled="true"> is set that this has the same function as <pf-tab disabled> and update the component accordingly? /cc @nikkimk

Right now the component becomes selectable.

@bennypowers
Copy link
Member Author

bennypowers commented Mar 4, 2024

The idea is that users should not set aria attrs on any tab elements. They should only and always use disabled when they need that feature

zeroedin
zeroedin reviewed Mar 4, 2024
View reviewed changes
zeroedin
zeroedin reviewed Mar 4, 2024
View reviewed changes
zeroedin
zeroedin reviewed Mar 4, 2024
View reviewed changes
@github-actions github-actions bot added the doc label Mar 5, 2024
@bennypowers bennypowers requested a review from zeroedin March 5, 2024 11:21
@bennypowers bennypowers enabled auto-merge (squash) March 5, 2024 13:02
zeroedin
zeroedin approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@zeroedin zeroedin left a comment

Choose a reason for hiding this comment

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

Given resolution of last remaining open comment

Lifted Granite Tall Mountains

nikkimk
nikkimk reviewed Mar 5, 2024
View reviewed changes
@bennypowers bennypowers merged commit d4e5411 into main Mar 6, 2024
@bennypowers bennypowers deleted the fix/tabs/controller-refactor branch March 6, 2024 08:17
@bennypowers bennypowers mentioned this pull request Mar 27, 2024
Closed
@zeroedin zeroedin mentioned this pull request May 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@nikkimk nikkimk nikkimk left review comments

@zeroedin zeroedin zeroedin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AT passed Automated testing has passed demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools

Projects

Workflow
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

TabsController should not make assumptions about host's DOM

4 participants

@bennypowers @zeroedin @nikkimk