-
Notifications
You must be signed in to change notification settings - Fork 28
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
CNV-29392: VM actions menu jumps when vms on list change state #1379
CNV-29392: VM actions menu jumps when vms on list change state #1379
Conversation
@avivtur: This pull request references CNV-29392 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
dfb99d2
to
2e5d722
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.
The code changes look good to me, make sense. I've tested them both in Chromium and Firefox browsers (up to date). In Chromium it works fine, however in Firefox I wasn't able to reach some actions in the drop down at all, for smaller browser window:
simplescreenrecorder-2023-06-29_14.33.49.mp4
IMHO we need to find a way to prevent scrolling back up automatically when trying to reach some actions at the end of the menu.
|
d572224
to
b02abe3
Compare
f877b46
to
091a9ac
Compare
@avivtur: This pull request references CNV-29392 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
091a9ac
to
092800b
Compare
@avivtur: This pull request references CNV-29392 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@upalatucci @hstastna thanks for reminding me that issue :) |
092800b
to
c12f3f4
Compare
Signed-off-by: Aviv Turgeman <[email protected]>
c12f3f4
to
af801b8
Compare
@avivtur: This pull request references CNV-29392 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@avivtur: This pull request references CNV-29392 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@upalatucci @hstastna @metalice |
Sounds reasonable to me. But as fixing bug in PF usually takes very long time, what would be cost of simply not using PF component (but some other one, maybe implemented by us) in this specific scenario? WDYT, Aviv? |
@hstastna I'd say it can take at least a day to make a new dropdown component (assuming I can make one based on menu component from PF, if not that can take more to copy all the css) |
Okay for me @hstastna |
Of course it depends on the zoom level. And unfortunately, on the size of the browser window, too. This PR improves the jumping issue a little bit, but still not fixed completely. I'd suggest to merge this PR as is, to contact PF folks to fix this issue in the component asap, with the link to the issue you've opened.
Yes, this one, too, please. However I am not sure how to achieve this. Hopefully you know :D |
Yes i also agree, lets merge, its working fine on chrome and in special cases in firefox some lines are flaky, a bug was open to PF so i think its enough for now. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avivtur, metalice 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 |
PF issue link: patternfly/patternfly-react#9326 |
/retest |
📝 Description
When appending the dropdown to the outer page, every re-render on that page will cause the dropdown to re-render and change location.
When using the Dropdown component from PF we can use the menuAppentTo prop to append the dropdown to the parent, however this presents a scrolling issue on the firefox browser, as the dropdown keeps getting focus and the scorlling would seem stuck. this is a PF issue that needs to be resolved as the focus is coming from the dropdown component.
🎥 Demo
Before:
vm-actions-menu-is-moving.mp4
After:
vm-actions-menu-is-moving-after.mp4