-
Notifications
You must be signed in to change notification settings - Fork 272
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
fix(ui5-view-settings-dialog): clicking on the radio button/checkbox works #10706
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.
Fixed. |
|
||
// Open the dialog and wait until it's visible | ||
cy.get("@vsd") | ||
.invoke("prop", "open", true) |
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.
Separate invoke from other commands.
cy.get("@vsd")
.invoke("prop", "open", true)
cy.get("@vsd")
.shadow()
.find("[ui5-dialog]")
.should("be.visible");
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.
Done
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 good to me!
Fixes the reported bug, the event detail seems as expected and the behavior is correct
🎉 This PR is included in version v2.7.0 🎉 The release is available on v2.7.0 Your semantic-release bot 📦🚀 |
View Settings Dialog - selection issues
Fixes
Background
Currently,
ui5-view-settings-dialog
binds to theitem-click
event ofui5-list
for all scenarios. However, this event is only fired if the user clicks on the item itself (its text). Therefore, if the user clicks on the radio button or checkbox instead, nothing happens (theitem-click
event is not fired).Solution
The fix is to bind to the
selection-change
event ofui5-list
instead, which is fired both when the user clicks on the text, and on the radio button/checkbox alike. For the step 1 of filters (pure list with no radio buttons/checkboxes the existingitemClick
event should be used, so no change there).Additionally, the
_setSelectedProp
function has been refactored - passing it the event object is an overkill, because it must extract the text from the selected item again. It's better to just pass the text (already extracted) directly.Note: some of the code in the test should be extracted to commands for the dialog/list components. This can be done independently after a discussion with the owners of these components.
closes: #10515