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.

Sign up for GitHub

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

Jump to bottom

Jquery enable disable checked calls conversion #1353

Merged
merged 2 commits into from
Jan 15, 2015
Merged

Jquery enable disable checked calls conversion #1353

merged 2 commits into from
Jan 15, 2015

Conversation

h-kataria
Copy link
Contributor

@martinpovolny @dclarizio please review.

Creating a new PR as i am unable to push my changes on to @bmclaughlin 's branch, closing original PR #1319

@h-kataria h-kataria mentioned this pull request Jan 13, 2015
Closed
dclarizio
dclarizio reviewed Jan 14, 2015
View reviewed changes
vmdb/app/assets/javascripts/cfme_application.js
@@ -319,8 +319,8 @@ function toggleConvertButtonToLink(button, url, toggle) {
// parms: button_div=<id of div with buttons to update>, override=<forced state>
function miqUpdateAllCheckboxes(button_div,override) {
miqSparkle(true);
if ($('masterToggle')) {
var state = $('masterToggle').checked;
if ($j('#masterToggle')) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@h-kataria The jQuery $j('#masterToggle') is not equivalent to the prototype $('masterToggle') is it? I thought we had to do something with the [0] array element to get back a false if the element id is not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dclarizio you are right i will have to fix that, i can check $j('#masterToggle').length > 0 to make sure element id exists in DOM.

@dclarizio
Copy link

@h-kataria While testing the retirement button, I hit two issues:

  • An existing (in master) issue is that when pressing the Remove button (the X), no transaction is sent up to clear the retirement date, but the text box is cleared on the screen. If Save is hit at this point, the retirement date is set even though the user meant to clear it. This may have been introduced when the observers where changed to jQuery, so maybe check prior to that to see if we need to open a 5.3 bug on this or not.
  • Remove button (the X) does not go away after the date is cleared. Most likely due to the first issue above since the server may be sending down the JS to hide it, but I'm not sure.

@h-kataria
Copy link
Contributor Author

@dclarizio issues found in retirement screen are related to HAML conversion of retire view, @AparnaKarve was planning to ping @skateman about that.

@h-kataria
Converted prototype -> jquery
64d91da 
- replace calls to checked
- split 'javascript_disable_field' method into 2 methods: javascript_disable_field & javascript_enable_field for more clarity.

ManageIQ#1148
@h-kataria h-kataria force-pushed the jquery_enable_disable_checked_calls_conversion branch from 8936711 to 64d91da Compare January 14, 2015 15:32
@miq-bot
Copy link
Member

miq-bot commented Jan 14, 2015

Checked commits h-kataria@3f2e81f .. h-kataria@64d91da with rubocop 0.27.1
4 files checked, 0 offenses detected
Everything looks good. ⭐

@h-kataria
Copy link
Contributor Author

@dclarizio please re-review.

@dclarizio
Copy link

@h-kataria I see the new JS functions, if we are not fixing the bugs I mentioned, can we get an issue opened to address them? Thx, Dan

@h-kataria
Copy link
Contributor Author

@dclarizio created #1384, assigned it to @skateman

dclarizio pushed a commit that referenced this pull request Jan 15, 2015
Merge pull request #1353 from h-kataria/jquery_enable_disable_checked…
2dd925f 
…_calls_conversion

Jquery enable disable checked calls conversion
@dclarizio dclarizio merged commit 2dd925f into ManageIQ:master Jan 15, 2015
@dclarizio dclarizio added this to the Sprint 18 Ending Jan 27, 2015 milestone Jan 15, 2015
@h-kataria h-kataria deleted the jquery_enable_disable_checked_calls_conversion branch January 20, 2015 19:07
@h-kataria h-kataria mentioned this pull request Feb 4, 2015
Closed
simaishi added a commit that referenced this pull request Jun 9, 2017
@simaishi
Merge pull request #1353 from lgalis/fine_add_missing_ownership_route…
e209a0f 
…_for_miq_template

[FINE] Add missing ownership route for miq template
(cherry picked from commit ef6694d)

https://bugzilla.redhat.com/show_bug.cgi?id=1448073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@h-kataria @dclarizio @miq-bot @Fryguy @bmclaughlin