Skip to content

Conversation

@kmcfaul
Copy link
Contributor

@kmcfaul kmcfaul commented Sep 20, 2022

What: Closes #7965

Adds remaining filter demos (attribute search, same select filter group, mixed select filter group, faceted filter).

Updates all filter demos to have the following:

  • Bulk select respects filtered state
  • Menu ids are unique between demos
  • Checkbox column space is maintained when there is an empty state
  • Tds are truncated to maintain column widths while filtering

While adding the last bullet I also noticed that no tooltip is added when Tds are truncated like Ths should I added that functionality as well.

Other updates in this PR:

  • Td: Added tooltip prop, and when using the truncate modifier, Td will attempt to make a default tooltip like Th
  • Th: Updated internal canDefault variable name to canMakeDefaultTooltip (matched in Td as well)
  • MenuToggle: Added missing ref spread to split button variant of the MenuToggle
  • Menu: added null check in createNavigableItems callback that can throw a console error if the menu closes due to the extraKeys handler

@patternfly-build
Copy link
Collaborator

patternfly-build commented Sep 20, 2022

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Had a few comments below (one nit, one clarifying question, and a couple issues I noticed).

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@kmcfaul these look great at a glance. But I'll defer to @mmenestr for a thorough design review.

@mmenestr
Copy link
Collaborator

mmenestr commented Sep 26, 2022

@kmcfaul looks great overall!! Only noticed one little bug in the last demo, where there should be a badge showing the number of selections - and it seems to work for the first attribute, but not for the second. As soon as you select anything from the second attribute, the badge disappears.

Screen.Recording.2022-09-26.at.11.44.53.AM.mov

@tlabaj tlabaj requested a review from mcoker September 28, 2022 20:58
Copy link
Collaborator

@wise-king-sullyman wise-king-sullyman left a comment

Choose a reason for hiding this comment

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

Looks good other than the badge bug in the faceted filter demo that was already mentioned, ready to approve once that's resolved.

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Sep 30, 2022

Fixed the faceted filter badge. @mmenestr @wise-king-sullyman

Not sure why but the check being inline wasn't quite executing right (it was only looking at the first condition), probably a syntax thing. Moving the check above to a const lets it work properly.

Copy link
Collaborator

@mmenestr mmenestr left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

I tried adding tooltip="tooltip text" and tooltip={<div>tooltip text</div>} to cells and it didn't trigger a tooltip - should that work or is this something else?

Sortable cells seem to be triggering 2 tooltips.
Screen Shot 2022-10-03 at 11 17 45 AM

And if a cell is sortable and has a help icon, when you mouse off the sort button and to the help button (but keep your mouse in the table cell), one of the cell tooltips stays open, then the help button tooltip shows.
Screen Shot 2022-10-03 at 11 19 08 AM

@kmcfaul
Copy link
Contributor Author

kmcfaul commented Oct 3, 2022

The tooltip prop is for specifying a custom tooltip when a tooltip is rendered (when there is the truncate modifier and the text length overflows). I don't think it forces the tooltip but I'm not sure. It should behave the same as the Th prop. @nicolethoen @mcoker

We may want to open a follow up for the extra tooltips as I'm not sure what's happening there at the moment. I added a tooltip to Td but I don't think that would be causing a Th to render a second tooltip.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM. Confirmed the double tooltip is a pre-existing issue. Here's an issue for that - https://github.com/patternfly/patternfly/issues/5136

@tlabaj tlabaj merged commit 599639a into patternfly:main Oct 4, 2022
@patternfly-build
Copy link
Collaborator

Your changes have been released in:

Thanks for your contribution! 🎉

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.

Filter demos: attribute search, filter groups, faceted

8 participants