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

Zero the toolbar count when asked #1058

Merged
merged 2 commits into from
Apr 20, 2017
Merged

Zero the toolbar count when asked #1058

merged 2 commits into from
Apr 20, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Apr 18, 2017

Before the toolbar refactor, miqSetButtons would set the right counts for toolbar.

This no longer happens and miqSetButtons is almost dead .. except it is still being called in all the places where we want to reset the counts.

So, adding a special condition to reset the toolbar count when count = 0.

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

(also added euwe/yes because this is also related to https://bugzilla.redhat.com/show_bug.cgi?id=1440118, together with #1054)


This needs more cleanup, and complete refactoring of miqUpdateButtons and miqUpdateAllCheckboxes and miqSetButtons (and related code), but for now .. this fixes the bug :).

When testing, the changes from #1054 are also needed to get consistent behaviour without extra clicking.

Fine BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1444035


EUWE: https://bugzilla.redhat.com/show_bug.cgi?id=1444178 and https://bugzilla.redhat.com/show_bug.cgi?id=1444037 - please backport together with #1095

himdel added 2 commits April 18, 2017 15:28
The angular toolbars no longer create an element with #foo_tb.

So any code that needs such a div to work, and doesn't get called when the element doesn't exist is dead :).

Removing
…th 0

before the toolbar refactor, miqSetButtons would set everything to the right count.

It is still being called in places where we need to update the toolbar count, but doesn't work most of the time because it no longer touches any toolbars.

This needs more refactoring to be clean, but for now, at least we should reset the toolbar to 0 when asked to :).

https://bugzilla.redhat.com/show_bug.cgi?id=1440142
@miq-bot
Copy link
Member

miq-bot commented Apr 18, 2017

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/b3b83700e7113a0c81de2951f32f429d828cce8e~...a6740381f364b7bf77637de0860f25bf4ff55a8b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@martinpovolny
Copy link
Member

@himdel, so I have to wait with this one until #1054 by @jzigmund is merged, but that one has failing specs so I will get back here tomorrow.

@himdel himdel added the blocker label Apr 19, 2017
@martinpovolny martinpovolny merged commit 39f6b87 into ManageIQ:master Apr 20, 2017
@martinpovolny martinpovolny added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 20, 2017
@himdel himdel deleted the toolbar-zero-bz1440142 branch April 20, 2017 10:15
simaishi pushed a commit that referenced this pull request Apr 20, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 78c7bb924d0a36bac3e2a22960a00ea642e0f40d
Author: Martin Povolny <[email protected]>
Date:   Thu Apr 20 10:34:07 2017 +0200

    Merge pull request #1058 from himdel/toolbar-zero-bz1440142
    
    Zero the toolbar count when asked
    (cherry picked from commit 39f6b872dc74fc10a3a21141d813089eaf53506f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444035

@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit b85c9d3438874faeee35b3638eb8592956258724
Author: Martin Povolny <[email protected]>
Date:   Thu Apr 20 10:34:07 2017 +0200

    Merge pull request #1058 from himdel/toolbar-zero-bz1440142
    
    Zero the toolbar count when asked
    (cherry picked from commit 39f6b872dc74fc10a3a21141d813089eaf53506f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444178
    https://bugzilla.redhat.com/show_bug.cgi?id=1444037

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