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

👻 Convert single/multi filter toolbar controls to use pf5 select #1669

Merged
merged 5 commits into from
Feb 21, 2024

Conversation

ibolton336
Copy link
Member

@ibolton336 ibolton336 commented Jan 19, 2024

@ibolton336 ibolton336 changed the title 👻 Begin converting filter toolbar controls to new pf select 👻 Convert single/multi filter toolbar controls to use pf5 select Jan 24, 2024
@ibolton336 ibolton336 force-pushed the mta-1872 branch 2 times, most recently from 3d68556 to f345edb Compare January 24, 2024 18:21
@ibolton336 ibolton336 marked this pull request as ready for review January 24, 2024 18:22
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

Some problems I found playing with the new filter toolbars:

  • Deselecting an item in a filter list doesn't work / each time you click the same entry, another chip is added

    • application inventory page, with multiple applications available
    • open the name filter dropdown with or without filter text
    • click to select a name
    • name cannot be unselected here, only by clicking the x in the name chips list
  • The filter type drop down does not auto-close

  • If the multiselect filter select drop down opens from the twistee, keyboard focus is left on the checkbox. But on selecting an item (space bar on the checkbox), focus goes back to the filter input box and any further attempt to select items with the spacebar actually types in the filter and changes the list. Only hitting enter will allow selecting a second item, but this also closes the dropdown.

For example:

screencast-localhost_9000-2024.02.13-13_24_54.mp4

Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

This review is just on the single select control SelectFilterControl. The control looks and works ok.

I have some suggestions to simplify the code in SelectFilterControl.

@ibolton336
Copy link
Member Author

Some problems I found playing with the new filter toolbars:

  • Deselecting an item in a filter list doesn't work / each time you click the same entry, another chip is added

    • application inventory page, with multiple applications available
    • open the name filter dropdown with or without filter text
    • click to select a name
    • name cannot be unselected here, only by clicking the x in the name chips list
  • The filter type drop down does not auto-close

  • If the multiselect filter select drop down opens from the twistee, keyboard focus is left on the checkbox. But on selecting an item (space bar on the checkbox), focus goes back to the filter input box and any further attempt to select items with the spacebar actually types in the filter and changes the list. Only hitting enter will allow selecting a second item, but this also closes the dropdown.

For example:

screencast-localhost_9000-2024.02.13-13_24_54.mp4

Cleaned things up a bit to more closely match this example https://www.patternfly.org/components/menus/select#multiple-typeahead-with-checkboxes & fixed most of the bugs you mentioned.

@ibolton336 ibolton336 force-pushed the mta-1872 branch 2 times, most recently from 284e089 to 6fd778a Compare February 15, 2024 15:38
@ibolton336 ibolton336 requested a review from sjd78 February 15, 2024 15:38
Copy link
Member

@sjd78 sjd78 left a comment

Choose a reason for hiding this comment

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

I've only looked at SelectFilterControl so far in this version of changes. I spent a bunch of time checking out the filter category selectOptions data against the PF5 SelectOption data. Looks like a small change could close the data gaps...

I'll have a look at the Multiselect behavior then code next.

- Cleanup the single select

- Disable if no selectable option

- Cleanup multiselect filter control

Signed-off-by: Ian Bolton <[email protected]>
@ibolton336 ibolton336 added the cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch. label Feb 21, 2024
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

ack

@ibolton336 ibolton336 merged commit 3de8f6b into konveyor:main Feb 21, 2024
6 checks passed
github-actions bot pushed a commit that referenced this pull request Feb 21, 2024
- Resolves:
   - https://issues.redhat.com/browse/MTA-1872
   - https://issues.redhat.com/browse/MTA-467

- Upgrade our usage of the PF select component to the pf 5 supported
version.

---------

Signed-off-by: ibolton336 <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
@ibolton336 ibolton336 deleted the mta-1872 branch February 21, 2024 19:36
ibolton336 added a commit that referenced this pull request Feb 21, 2024
…) (#1694)

- Resolves:
   - https://issues.redhat.com/browse/MTA-1872
   - https://issues.redhat.com/browse/MTA-467

- Upgrade our usage of the PF select component to the pf 5 supported
version.

---------

Signed-off-by: ibolton336 <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>

Signed-off-by: ibolton336 <[email protected]>
Signed-off-by: Ian Bolton <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Ian Bolton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.3 This PR should be cherry-picked to release-0.3 branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants