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

Handle change event for checkboxes, not click #10337

Merged
merged 6 commits into from Aug 18, 2016
Merged

Handle change event for checkboxes, not click #10337

merged 6 commits into from Aug 18, 2016

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Aug 9, 2016

Right now many of our checkboxes deal with the click event, which is only triggered when clicking the checkbox by mouse, and may not be triggered when clicking the associated label, using keyboard, screenreader, etc..

Especially they weren't triggered by setting .prop('checked', truthy) in miqUpdateAllCheckboxes..

Updating to use the change event instead..

https://bugzilla.redhat.com/show_bug.cgi?id=1343659

@himdel himdel changed the title [WIP] Handle change event for checkboxes, not click Handle change event for checkboxes, not click Aug 10, 2016
@himdel himdel removed the wip label Aug 10, 2016
@mzazrivec mzazrivec self-assigned this Aug 16, 2016
@mzazrivec
Copy link
Contributor

@himdel For some reason, with your changes, when I navigate to:
Compute ->
Infrastructure ->
Virtual Machines ->
grid or tile view, I select random amount of vms (mouse click), then click on 'Check All' ->
Configuration ->
Set ownership ->
I see only the explicitly selected vms, not all of them.

@himdel
Copy link
Contributor Author

himdel commented Aug 16, 2016

@mzazrivec .. ahh, thanks, now, I'm seeing it too.. investigating..

@miq-bot
Copy link
Member

miq-bot commented Aug 16, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

..because :servers_all is never set in :edit
just render the cases to make the conditions simpler, and remove unused override
makes little sense to fix in this PR and may be nontrivial because onchange gets generated for the newly clicked radio *and* for the previously-checked radio
This is so that checkbox handlers do the correct thing even when the checkbox was checked by keyboard, etc.

https://bugzilla.redhat.com/show_bug.cgi?id=1343659
@himdel
Copy link
Contributor Author

himdel commented Aug 17, 2016

OK, this fixes the bug.. I'm just not quite sure yet about all the places I've added triggering the change event to are really supposed to have it yet.. (8656dc8)

@himdel
Copy link
Contributor Author

himdel commented Aug 17, 2016

.. pretty sure I've found the right combination .. triggering change on every checking that's not done from the backend and that does not change a check-all button itself.

@miq-bot
Copy link
Member

miq-bot commented Aug 17, 2016

Checked commits https://github.com/himdel/manageiq/compare/c105919255a1ce69da394b9e699660bd21236327~...461c2ea4c1427da2e9ec04f8f2f5be773e91baee with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
12 files checked, 0 offenses detected
Everything looks good. 🍪

@mzazrivec mzazrivec added this to the Sprint 45 Ending Aug 22, 2016 milestone Aug 18, 2016
@mzazrivec mzazrivec merged commit 686922f into ManageIQ:master Aug 18, 2016
@himdel himdel deleted the bz1343659-checkboxes branch August 18, 2016 13:34
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.

None yet

4 participants