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

Fixed code to start spinner when selecting values in drop down. #8823

Merged
merged 1 commit into from May 20, 2016

Conversation

h-kataria
Copy link
Contributor

  • Removed code that was setting options.beforeSend and options.complete based upon values from $(this).attr('data-miq_sparkle_on') & $(this).attr('data-miq_sparkle_off'). With new bootstrap drop downs we no longer set data-miq_sparkle_on & data-miq_sparkle_off, values for beforeSend & complete are being passed in along with options to miqSelectPickerEvent JS function from views.
  • Added spec test to verify fix

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

@dclarizio @himdel please review.

This code confusion started in commit a628815 in https://github.com/ManageIQ/manageiq/pull/8316 where changes were instroduced to turn spinner on/off based upon $(this).attr('data-miq_sparkle_on')/$(this).attr('data-miq_sparkle_off') this code was further refactored/changed in latest commits assuming that spinner should be turned on/off solely based upon $(this).attr('data-miq_sparkle_on')/$(this).attr('data-miq_sparkle_off').

@h-kataria h-kataria force-pushed the fix_for_spinners_in_pull_downs branch from 086400a to 101192a Compare May 19, 2016 16:54
@h-kataria h-kataria added the wip label May 19, 2016
@h-kataria h-kataria changed the title Fixed code to start spinner when selecting values in drop down. [WIP] - Fixed code to start spinner when selecting values in drop down. May 19, 2016
@h-kataria h-kataria force-pushed the fix_for_spinners_in_pull_downs branch from 2810adf to 101192a Compare May 19, 2016 17:04
- Changed code to set options.beforeSend and options.complete using $(this).attr('data-miq_sparkle_on') & $(this).attr('data-miq_sparkle_off') only if $(this).attr('data-miq_sparkle_on') & $(this).attr('data-miq_sparkle_off') were set otherwise use the values that were sent with JS function call.
- Added spec tests to verify fix

https://bugzilla.redhat.com/show_bug.cgi?id=1337475
@h-kataria h-kataria force-pushed the fix_for_spinners_in_pull_downs branch from 101192a to 2a127d1 Compare May 19, 2016 17:34
@h-kataria h-kataria changed the title [WIP] - Fixed code to start spinner when selecting values in drop down. Fixed code to start spinner when selecting values in drop down. May 19, 2016
@h-kataria h-kataria removed the wip label May 19, 2016
@h-kataria
Copy link
Contributor Author

@dclarizio @himdel ready for review. With this fix it should work if data-miq_sparkle_on/'data-miq_sparkle_off attributes were set for a drop down and also if beforeSend/complete were passed in as options to miqSelectPickerEvent function call. We should cleanup the code to go with setting these single way, otherwise iti gets confusing and leads to errors.

@miq-bot
Copy link
Member

miq-bot commented May 19, 2016

Checked commit h-kataria@2a127d1 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
0 files checked, 0 offenses detected
Everything looks good. 🍪

@h-kataria
Copy link
Contributor Author

This PR should be merged once #8828 is fixed.

@himdel
Copy link
Contributor

himdel commented May 20, 2016

Aaah, those were overriding the beforeSend and complete that are being passed in, right. Makes sense! :) (and #8828 is fixed in #8839).

@dclarizio dclarizio merged commit a951789 into ManageIQ:master May 20, 2016
@dclarizio dclarizio deleted the fix_for_spinners_in_pull_downs branch May 20, 2016 14:39
@dclarizio dclarizio added this to the Sprint 41 Ending May 30, 2016 milestone May 20, 2016
chessbyte pushed a commit that referenced this pull request May 20, 2016
Fixed code to start spinner when selecting values in drop down.
(cherry picked from commit a951789)
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

5 participants