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

Toolbar buttons converted to use generic class with disable #10366

Conversation

romanblanco
Copy link
Member

@romanblanco romanblanco commented Aug 10, 2016

Continuation of #10276.

Purpose or Intent

Include buttons that share the same logic for skip? method into one generic class GenericFeatureButtonWithDisable

Links

Parent issue: #6259
Related issue: #6554

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2016

@romanblanco unrecognized command 'technical', ignoring...

Accepted commands are: add_label, remove_label, rm_label, assign, set_milestone

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2016

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

@romanblanco romanblanco force-pushed the convert_tb_button_to_generic_with_disable branch from 9e4ed26 to aee0391 Compare August 10, 2016 13:59
@romanblanco
Copy link
Member Author

@martinpovolny @PanSpagetka can you review?

@miq-bot miq-bot removed the wip label Aug 10, 2016
@romanblanco romanblanco changed the title [WIP] Toolbar buttons converted to use generic class with disable Toolbar buttons converted to use generic class with disable Aug 10, 2016
@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2016

Checked commits romanblanco/manageiq@6f57be8~...aee0391 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 👍

@martinpovolny
Copy link
Member

@dclarizio wrote:

just noticed that disabled hover text is showing for some buttons, so it is not broken everywhere . . . saw it NOT working in VMs area
seems ok in Hosts area

That matched the area that has been worked on.

This is the button that shall handle the generic behavior where button changes the hover text when enabled. Right?

Please, make sure this is fixed.

ping @PanSpagetka : please, test together making sure that all the hover texts, disable/enable show/hide remain functioning ;-)

@dclarizio
Copy link

@PanSpagetka did some testing with this branch. Compared to current master, I see now that ONLY valid power operation buttons for a VM are shown and active, which is correct.

I noticed that the disabled Configuration / Extract Running Processes button does show the reason, on both master and this branch.

However, I tried several VMs and both the Set Retirement Date and Retire this VM buttons in Lifecycle are disabled with no reason. This is the same on master and on this branch, but I would think we can always do these functions, so maybe that broke with an earlier PR. Either way, if a button is disabled, the hover text should show the reason, so I'm not sure if the bug is that they are disabled or that they don't show a reason for being disabled.

@romanblanco
Copy link
Member Author

romanblanco commented Aug 11, 2016

@martinpovolny wrote:

@dclarizio wrote:

just noticed that disabled hover text is showing for some buttons, so it is not broken everywhere . . . saw it NOT working in VMs area
seems ok in Hosts area

That matched the area that has been worked on.

Yes. @PanSpagetka has found my bug in recently converted buttons (VM buttons). The problem was, that I didn't set self[:enabled] attribute in calculate_properties, only self[:title], causing that buttons were always enabled, and only had the title, when they should've been disabled.

That issue is fixed in @PanSpagetka's new Generic class, where he sets both attributes, and by this conversion, the buttons are using the new Generic class, so it should work properly now.


@martinpovolny wrote:

This is the button that shall handle the generic behavior where button changes the hover text when enabled. Right?

Right.

There are still some buttons that are broken, but I've not converted them, as they do not have the same pattern as these.

@martinpovolny martinpovolny added this to the Sprint 45 Ending Aug 22, 2016 milestone Aug 11, 2016
@martinpovolny martinpovolny merged commit 3ebedcc into ManageIQ:master Aug 11, 2016
@romanblanco romanblanco deleted the convert_tb_button_to_generic_with_disable branch August 11, 2016 10: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.

4 participants