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(tabs): implement tabs controller #2577

Merged
merged 52 commits into from
Dec 4, 2023

Conversation

zeroedin
Copy link
Collaborator

@zeroedin zeroedin commented Aug 11, 2023

What I did

  1. Created tabs-controller in pfe-core
  2. Implemented tabs-controller in pf-tabs no longer inherits from BaseTabs
  3. Improved overflow functionality
  4. Added dynamic tab creation support
  5. Removed logic from BaseTabs and created TabsController reverted
  6. Removed BaseTabs, BaseTab and BasePanel, moved remaining logic to pf-tab and pf-tab-panel reverted

Closes #2575

TODO:

  • Answer question: Should we reintroduce BaseClasses with an implementation using the TabsController? ( I have this saved as a stash atm, need to have a conversation regarding this approach and breaking changes in general). If we reimplement BaseTabs using TabsController should pf-tabs extend this BaseClass or should it extend from LitElement given discussion here: Planning for Version 3.0: Removal of Base Classes #2578 (We are removing BaseClasses in 3.0, so no need to reimplement the tabs controller here)
  • Answer question: Should overflow controller be integrated into TabsController? What constitutes the minimum for what TabsController contains? My initial thought was that overflow isn't tabs specific and could envision tabs without the overflow option. However, RTI is integrated in the TabsController as it is integral to the accessibility of tabs and can be baked in. (After a couple discussions on this, overflow is an additive feature and it should stay abstracted from the tabs controller).
  • Fix bug triggering an overflow check when tabs are added dynamically.
  • Fix bug in tabs not activated by keyboard navigation not updating RTI.
  • Add tabs-controller.js to core package exports
  • Add changeset
  • Moved TabsController to core

Testing Instructions

  1. View Deploy Preview demo
  2. View Deploy Preview dynamic tabs demo

Notes to Reviewers

feat(tabs): implement tabs controller
@zeroedin zeroedin added the functionality Functionality, typically pertaining to the JavaScript. label Aug 11, 2023
@zeroedin zeroedin self-assigned this Aug 11, 2023
@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

🦋 Changeset detected

Latest commit: 8f54b25

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 the styles An issue or PR pertaining only to CSS/Sass label Aug 11, 2023
@github-actions github-actions bot added the work in progress POC / Not ready for review label Aug 11, 2023
@github-actions github-actions bot added the AT passed Automated testing has passed label Aug 11, 2023
@github-actions github-actions bot added the tests Related to testing label Aug 11, 2023
@github-actions github-actions bot added the demo Updating demo pages label Aug 11, 2023
@netlify
Copy link

netlify bot commented Aug 14, 2023

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit 4fd993d
😎 Deploy Preview https://deploy-preview-2577--patternfly-elements.netlify.app/

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

@zeroedin zeroedin changed the base branch from main to feat/3.0/collect-prs August 25, 2023 17:41
@zeroedin
Copy link
Collaborator Author

@bennypowers assuming we want to hold this one off till after 2.5 release?

@bennypowers
Copy link
Member

what's holding it back?

@zeroedin
Copy link
Collaborator Author

what's holding it back?

Maybe I misunderstood, but thought we are holding off on all changes that remove base classes till after 2.5.0?

@bennypowers
Copy link
Member

bennypowers commented Nov 15, 2023

if this doesn't change the public api of the base class, it can go in 2.5.0. then we can get a leg-up on implementing in rhds

@zeroedin
Copy link
Collaborator Author

zeroedin commented Nov 15, 2023

if this doesn't change the public api of the base class, it can go in 2.5.0. then we can get a leg-up on implementing in rhds

This PR deletes the base class completely.

@bennypowers
Copy link
Member

it doesn't have to though.. you could just keep the base class file around, or reimplement tabs in that base class using the controller

@zeroedin
Copy link
Collaborator Author

it doesn't have to though.. you could just keep the base class file around, or reimplement tabs in that base class using the controller

Ok after exploring believe leaving the base class around is the best option here. Implementation of the tabs-controller into the Base class still causes breaking changes due to other code reorg. I'll revert the removal of the Base classes here, and can follow up with their removal.

@zeroedin
Copy link
Collaborator Author

@bennypowers if we have time next week, lets pair on this just to comb it over to make sure I haven't misrepresented or misplaced anything.

Copy link
Member

@bennypowers bennypowers left a comment

Choose a reason for hiding this comment

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

last couple of bits. almost there

.changeset/hip-bags-live.md Outdated Show resolved Hide resolved
.changeset/tabs-controllers-modified.md Outdated Show resolved Hide resolved
elements/pf-tabs/pf-tab.css Outdated Show resolved Hide resolved
elements/pf-tabs/pf-tabs.css Outdated Show resolved Hide resolved
elements/pf-tabs/pf-tabs.ts Show resolved Hide resolved
@zeroedin zeroedin merged commit 9aee24e into feat/3.0/collect-prs Dec 4, 2023
12 checks passed
@zeroedin zeroedin deleted the feat/tabs-controller branch December 4, 2023 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed 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 work in progress POC / Not ready for review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Abstract Tabs functionality from BaseTabs to a Controller
2 participants