search

patternfly / patternfly-elements

PatternFly Elements. A set of community-created web components based on PatternFly design.
https://patternflyelements.org/
MIT License
375 stars 89 forks source link

fix(core)!: add getItems and getItemsContainer to RTI constructor #2678

Closed bennypowers closed 5 months ago

bennypowers commented 5 months ago

Closes #2644

What I did

  1. add getItems and getItemContainer option functions to RTIC, update call sites
  2. make updateItems parameter default to the result of getItems

Testing Instructions

  1. units, manual keyboard testing for rti elements
changeset-bot[bot] commented 5 months ago

🦋 Changeset detected

Latest commit: c294423d4d3f19110365fd8a693042a165cde5e0

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

This PR includes changesets to release 2 packages | Name | Type | | -------------------- | ----- | | @patternfly/pfe-core | Major | | @patternfly/elements | 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

netlify[bot] commented 5 months ago

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit e250a6e3aa27660a841e1222b9328ea7417d119e
Deploy Preview https://deploy-preview-2678--patternfly-elements.netlify.app/

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

zeroedin commented 5 months ago

Tabs aren't loading correctly with change to RTI initialization. Going to dig into this a bit further to look for a fix.

OK, I had to do a double take here. Some what related by not entirely to the above error mainly through attempted discovery. I was trying to figure out why some of the changes I had made to tabs prior were not present here. For whatever reason we collected those on this branch here:

https://github.com/patternfly/patternfly-elements/tree/feat/3.0/collect-prs

That branch was never merged into main now so those changes were lost to the current timeline. We need to get that merged back in. I have vague recollection of wanting to side track these changes for 2.5, but I believe they need merged into main for 3.0 now.

New PR opened to merge those tabs changes in main, then we should merge main back here. https://github.com/patternfly/patternfly-elements/pull/2679

bennypowers commented 5 months ago

@zeroedin merged in the linked branch, PTAL

zeroedin commented 5 months ago

That branch was never merged into main now so those changes were lost to the current timeline. We need to get that merged back in. I have vague recollection of wanting to side track these changes for 2.5, but I believe they need merged into main for 3.0 now.

New PR opened to merge those tabs changes in main, then we should merge main back here. #2679

Ok we'll have to take a look at the TabsController implementation of the new RTI initialization now. Looks like you had already anticipated this change:

// TODO(bennypowers): adjust to fit, in or after #2570
this.#tabindex.initItems(this._tabs, this.#element);

If I have the opportunity tonight to take a look at this ill push some code here otherwise first in the morning.

zeroedin commented 5 months ago

@nikkimk definitely double check keyboard navigation here, but things seem to be working as expected for me.

bennypowers commented 5 months ago

checked with @nikkimk. we'll merge this even though tests in select are failing and come back to fix those in #2677