-
Notifications
You must be signed in to change notification settings - Fork 163
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
Fixes Image selection dropdown scroll issue #2696
Fixes Image selection dropdown scroll issue #2696
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok a couple of things here:
Have you tested wether this fix the issue? I would think it's still happening if we still have the css.
We should have coverage with mockup data that might be recreating the issue, we can talk about that async if you want.
Is this refactor gonna help other instances of this? Maybe we should just fix the issue of the height and do the refactor in another issue OR just do the refactor in all the instances, but it's not a great idea having both components.
2f5bea2
to
ec8420b
Compare
ec8420b
to
781bd2f
Compare
0b798a8
to
5ed154f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
@lucferbux Fixed all test failures arising due to the migration. Added a new test to test scrollable dropdown selection as requested. Even manually tested the views on all the pages, seems to work fine. The CSS file you mentioned seems to fix some width issues instead of height and it's required coz the extra padding persists if we remove that CSS. This is ready for review. |
5ed154f
to
c15001f
Compare
/lgtm Readding the label coz the last force-push only included a Cypress function name change from |
frontend/src/__tests__/cypress/cypress/support/commands/application.ts
Outdated
Show resolved
Hide resolved
c15001f
to
364c301
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Had to check the part of testId
prop, but I think that's the only solution we can go for now, since that's how patternfly recommends using the testing id.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dpanshug, lucferbux 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 |
Closes: https://issues.redhat.com/browse/RHOAIENG-3412
Description
Fixes the image selection dropdown scroll issue. Upgraded
SimpleDropdownSelect
component to PF5.Screen.Recording.2024-04-10.at.7.23.30.PM.mov
How Has This Been Tested?
Try creating a workbench after creating many custom images using the
Notebook images
section in theSettings
section in the Dashboard. You should see a scroll bar while trying to select images in theImage selection
, which doesn't break the layout.Test Impact
Fixed failing tests due to the PF5 migration. Added new tests for scrollable dropdown selection
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main