-
Notifications
You must be signed in to change notification settings - Fork 356
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
Rework dual list #5228
Rework dual list #5228
Conversation
f985806
to
1eccf08
Compare
@miq-bot add-reviewer @rvsia |
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.
Looks great! 👍
I made some changes in my catalog-form PR, like adding paddingBottom to lists (because of buttons), could you please converted them here? Thanks!
CTRL+LeftClick on mutliple options doesn't work. 😿
And I think you should implement at least move all to right, it doesn't seem as a big change and it will be probably in the next converted form. 🔢 (MoveAllToLeft is not probably anywhere, it was just a bonus. 🎁 😄 )
b9dee64
to
bcf0e8f
Compare
Reason why it is not working, is because its expects array of strings. You are comparing array of numbers and strings which will fail. Right now values has to be always strings. If we ever need to changes this to support numbers objects etc, we will write custom equal function. Right now its not needed. |
bcf0e8f
to
54aae3f
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.
Tested in UI and everything looks great! 🥇
Checked commits Hyperkid123/manageiq-ui-classic@5fa7864~...54aae3f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Dual list was not proplely updating form state, specifically
pristine
/dirty
states. This was cause due to react refs and async nature of executing events via refs. Proposed solution which addstimeout
to manually triggeronBlur
andonFocus
events is future proof and will probably result in more bugs down the line.Now the component uses purely React state without any refs which will ensure synchronous behavior.