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

Update depricated PF multiselect component #2975

Merged

Conversation

ashley-o0o
Copy link
Contributor

@ashley-o0o ashley-o0o commented Jul 2, 2024

Closes: RHOAIENG-421

Description

This PR updates the deprecated Patternfly 4 select component with the PF5 multi select component

How Has This Been Tested?

This is tested with npm run test as well as on localhost, and using the Cypress tests in userManagement.cy.ts

Test Impact

No new tests were added as there are no functionality changes

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot requested review from mturley and pnaik1 July 2, 2024 19:40
@christianvogt
Copy link
Contributor

  • When I first click into the input field, my focus is lost and I need to click again before I can type.
  • After using the keyboard to highlight an item in the dropdown, pressing enter no longer selects / deselects that item.
  • After focusing the text input with the keyboard, pressing an arrow key no longer opens the menu. This behavior works in the PF example.
  • Previously I could not focus the arrow dropdown button. This made sense since the menu is controlled by focusing on the input field. Now I can also tab to the toggle button. The toggle button should not be focusable as per the PF example.

frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
frontend/src/components/MultiSelection.tsx Outdated Show resolved Hide resolved
@jeff-phillips-18
Copy link
Contributor

  • Previously I could not focus the arrow dropdown button. This made sense since the menu is controlled by focusing on the input field. Now I can also tab to the toggle button. The toggle button should not be focusable as per the PF example.

This is a PF issue. Though it works on the documentation page, if you try the code sandbox it does not and the toggle button is in the tab order for focus.

@ashley-o0o ashley-o0o force-pushed the 421/replace-PF-select branch 5 times, most recently from 0d5475c to bb99222 Compare July 9, 2024 17:55
@openshift-ci openshift-ci bot added the lgtm label Jul 9, 2024
@jeff-phillips-18
Copy link
Contributor

/lgtm cancel

@openshift-ci openshift-ci bot removed the lgtm label Jul 9, 2024
@jeff-phillips-18 jeff-phillips-18 self-requested a review July 9, 2024 21:49
@ashley-o0o ashley-o0o force-pushed the 421/replace-PF-select branch 2 times, most recently from d7ca5ed to ce12204 Compare July 10, 2024 20:02
Copy link
Contributor

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 11, 2024
@jeff-phillips-18 jeff-phillips-18 self-requested a review July 11, 2024 19:35
@jeff-phillips-18
Copy link
Contributor

/lgtm cancel

@openshift-ci openshift-ci bot removed the lgtm label Jul 11, 2024
@ashley-o0o ashley-o0o force-pushed the 421/replace-PF-select branch 3 times, most recently from ba55a53 to 0d16a4f Compare July 15, 2024 16:15
Copy link
Contributor

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 16, 2024
@christianvogt
Copy link
Contributor

/approve

Copy link
Contributor

openshift-ci bot commented Jul 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 7f56055 into opendatahub-io:main Jul 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants