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

Do not enable disabled button when items are selected #1031

Merged
merged 3 commits into from
Apr 13, 2017
Merged

Do not enable disabled button when items are selected #1031

merged 3 commits into from
Apr 13, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Apr 13, 2017 •
edited by himdel
Loading

Description

Currently the :onwhen property override the disabled? method of a button. If a disabled button has the :onwhen set, it will become enabled if user select an item from the list.

Currently the disabled? method is not called if self[:enabled].nil? is false, so the @error_message variable is not being set, and does not replace the :title.

This PR:
a. Remove the :onwhen property of a disabled item
b. Calls the disabled? method, to set the @error_message variable

:enable disabled? => :enabled :title :onwhen
true true/false true unchenged unchanged
false true false error nil
false false false unchenged unchanged
nil true false error nil
nil false true unchenged unchanged

Screenshots

Bug
screenshot-localhost 3000-2017-04-13-10-54-21

Fix
screenshot from 2017-04-13 11-04-53

Fix when disabled? is false
screenshot from 2017-04-13 11-18-40

Duplicate of:
#943
Fixes a bug related to this BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1438248

Closes #943

@yaacov
Copy link
Member Author

yaacov commented Apr 13, 2017

@miq-bot add_label compute/containers, bug

@simon3z @cben @zeari @enoodle @himdel @vecerek @romanblanco please review

@yaacov yaacov force-pushed the do-not-enable-a-disabled-button-when-items-are-selected branch 7 times, most recently from f6a6db2 to 13d918b Compare April 13, 2017 12:22
@yaacov yaacov force-pushed the do-not-enable-a-disabled-button-when-items-are-selected branch from 13d918b to 1b20a94 Compare April 13, 2017 12:26
romanblanco
romanblanco suggested changes Apr 13, 2017
View reviewed changes
app/helpers/application_helper/button/basic.rb Outdated
enabled = !disabled?
self[:enabled] = enabled if self[:enabled].nil?

unless self[:enabled] || enabled
Copy link
Member

Choose a reason for hiding this comment

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

self[:enabled].nil? would be better here, so it's clear, we are testing it for nil presence.

Copy link
Member Author

@yaacov yaacov Apr 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

:enable disabled? :enabled :title :onwhen
true true/false unchanged unchenged unchanged
false true false error nil
false false false unchenged unchanged
nil true false error nil
nil false true unchenged unchanged

@romanblanco
we change :enable only if it's nil
but we change :title and :onwhen on nil or false

but yes it's not nice, I can't change is to .nil? but can we do something else to make it better ?
my main problem is the :enable == false is not really false it's false unless :onwhen is true :-(

romanblanco
romanblanco suggested changes Apr 13, 2017
View reviewed changes
app/helpers/application_helper/button/basic.rb Outdated
def calculate_properties
self[:enabled] = !disabled? if self[:enabled].nil?
self[:title] = @error_message if @error_message.present?
enabled = !disabled?
Copy link
Member

Choose a reason for hiding this comment

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

The changes are nice! But the code is still a bit unreadable, can you add some comments here?

# returns enabled state and sets @error_message

app/helpers/application_helper/button/basic.rb Outdated
self[:enabled] = enabled if self[:enabled].nil?

unless self[:enabled] || enabled
self[:onwhen] = nil
Copy link
Member

Choose a reason for hiding this comment

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

# button will be visible and disabled, but won't be possible to enable it by javascript

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 on it, it is really unreadable now, @enoodle just comment on it too :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

@romanblanco added some comments PTAL

@romanblanco romanblanco mentioned this pull request Apr 13, 2017
Closed
@yaacov yaacov force-pushed the do-not-enable-a-disabled-button-when-items-are-selected branch from c1db244 to 8f7dcd3 Compare April 13, 2017 12:51
@simon3z
Copy link
Contributor

simon3z commented Apr 13, 2017

@dclarizio @h-kataria since this is core can you take care of the proper labels/BZs for which this is targeted to?

@romanblanco
Copy link
Member

@miq-bot add_label toolbars

@miq-bot
Copy link
Member

miq-bot commented Apr 13, 2017

Checked commits yaacov/manageiq-ui-classic@1b20a94~...36d1ae3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🏆

romanblanco
romanblanco approved these changes Apr 13, 2017
View reviewed changes
Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Looks good now 👍 🌞

@himdel himdel self-assigned this Apr 13, 2017
@himdel himdel added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 13, 2017
@himdel himdel merged commit ccd29a5 into ManageIQ:master Apr 13, 2017
simaishi pushed a commit that referenced this pull request Apr 13, 2017
@himdel @simaishi
Merge pull request #1031 from yaacov/do-not-enable-a-disabled-button-…
495636a 
…when-items-are-selected

Do not enable disabled button when items are selected
(cherry picked from commit ccd29a5)

https://bugzilla.redhat.com/show_bug.cgi?id=1440566
@simaishi
Copy link
Contributor

simaishi commented Apr 13, 2017 •
edited
Loading

Fine backport details:

$ git log -1
commit 495636aa989dcf765ae18a2a233d93208ee87682
Author: Martin Hradil <himdel@seznam.cz>
Date:   Thu Apr 13 13:48:41 2017 +0000

    Merge pull request #1031 from yaacov/do-not-enable-a-disabled-button-when-items-are-selected
    
    Do not enable disabled button when items are selected
    (cherry picked from commit ccd29a54b14089e63774f706b58b00434c01b802)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1440566

@simaishi simaishi removed the fine/yes label Apr 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanblanco romanblanco romanblanco approved these changes

Assignees

@himdel himdel

Labels
bug compute/containers fine/backported toolbars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yaacov @simon3z @romanblanco @miq-bot @simaishi @himdel