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

ToolbarController#setClickHandler - don't handle click for disabled toolbar items #2884

Merged
merged 1 commit into from
Dec 1, 2017
Merged

ToolbarController#setClickHandler - don't handle click for disabled toolbar items #2884

merged 1 commit into from
Dec 1, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Nov 30, 2017

clicking on a disabled toolbar item doesn't really do anything in miqToolbarOnClick, but the itemClicked rx message still got sent, causing the paging toolbar to disappear (by calling ReportDataController#setExtraClasses).

Changing to only send the event when a visible non-disabled button is clicked.

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

Cc @karelhala

…oolbar items

clicking on a disabled toolbar item doesn't really do anything in `miqToolbarOnClick`, but the `itemClicked` rx message still got sent, causing the paging toolbar to disappear (by calling `ReportDataController#setExtraClasses`).

Changing to only send the event when a visible non-disabled button is clicked.

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

himdel commented Nov 30, 2017

(the diff looks ugly because there was an off-by-one formatting - the code was indented at an odd (ie. not even) level of indentation - use whitespace-insensitive diff)

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

Good catch. However I don't like empty returns, but here it's understandable. So good to merge.

@mzazrivec mzazrivec self-assigned this Dec 1, 2017
@mzazrivec mzazrivec added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 1, 2017
@mzazrivec mzazrivec merged commit d27872b into ManageIQ:master Dec 1, 2017
@himdel himdel deleted the toolbar-pagination-bz1518929 branch December 1, 2017 13:33
simaishi pushed a commit that referenced this pull request Dec 1, 2017
ToolbarController#setClickHandler - don't handle click for disabled toolbar items
(cherry picked from commit d27872b)

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

simaishi commented Dec 1, 2017

Gaprindashvili backport details:

$ git log -1
commit fd10f0168dea46fc14854d0e1c877789e6a1a3b8
Author: Milan Zázrivec <mzazrivec@redhat.com>
Date:   Fri Dec 1 10:35:21 2017 +0100

    Merge pull request #2884 from himdel/toolbar-pagination-bz1518929
    
    ToolbarController#setClickHandler - don't handle click for disabled toolbar items
    (cherry picked from commit d27872b0293ed8d57813e7c75c72ed243a7d3908)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1519841

@himdel
Copy link
Contributor Author

himdel commented Dec 20, 2017

Seems like @nimrodshn found a proper fix in #3062 :).

But maybe we should still keep this as well, since doing anything when clicking disabled buttons is probably still wrong.

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