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

Remove paging (not paging_div) after leaving GTL screens #3262

Merged
merged 4 commits into from
Jan 18, 2018
Merged

Remove paging (not paging_div) after leaving GTL screens #3262

merged 4 commits into from
Jan 18, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jan 17, 2018

  1. Go to Ops > Diagostics > server detail > Workers.
  2. see paging toolbar there
  3. switch to Settings accordion
    before: you still see paging there, and form buttons
    after: you only see form buttons

Before the GTL rewrite, paging components used to be wrapped in #paging_div > #pc_div_1 > the toolbar.

Thus any code that tries to hide pc_div_1 used to hide the paging toolbar but keep #paging_div (used for form buttons as well).

This is now dead code, but looks like it's still needed when transitioning from a GTL screen (thus paging_div is there, and contains miq-pagination), to a form screen (thus paging_div doesn't get removed, but needs to only have form buttons).

Showing pc_div_1 is probably completely obsolete, because the GTL code does that by itself.

How removing paging is implemented - we simply call setExtraClasses(), used in other places (like clicking a toolbar) to remove the GTL classes. The complication is, this needs to work even without a running GTL instance - so wrapping the function in miqGtlSetExtraClasses to make it available without a gtl instance.

@karelhala @martinpovolny please review

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

…a ReportController instance

on screens without GTL, we may still need to clean the paging toolbar

setExtraClasses (without params) does the right thing .. but assumes an instance

created a function that runs it with a fake instance

https://bugzilla.redhat.com/show_bug.cgi?id=1512957
…o clean up after GTL

this is for ajax transitions from gtl screens to screens without gtl but with paging_div

this should probably replace `.hide('pc_div_1')` everywhere
…:pc_div_1)

pc_div_#(number) used to be a div under paging_div but above the pagination code
this was removed with the GTL refactor

thus, attempts to show/hide pc_div_1 are dead code, but indicate that those were the places where paging was being hidden

replaced by explicit remove_paging

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1512957
pc_div_.. used to wrap the paging under paging_div, before the GTL rewrite

replaced hiding it by remove_paging,
removed code showing it (because GTL automatically creates it anyway)
@miq-bot
Copy link
Member

miq-bot commented Jan 17, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/69419c02ce2bcd596e5539ed0e42403e100df447~...a7838821342b0ab0c28ebf65b2789e4435c91f5e with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
12 files checked, 0 offenses detected
Everything looks fine. ⭐

@h-kataria
Copy link
Contributor

@karelhala @martinpovolny can you please review/merge this blocker PR.

@dclarizio dclarizio self-assigned this Jan 18, 2018
@dclarizio
Copy link

1. Go to Ops > Diagostics > server detail > Workers.
2. see paging toolbar there
3. switch to Settings accordion
before: you still see paging there, and form buttons
after: you only see form buttons

Testing in the UI, following these steps:

  • For 2, I do not see paging toolbar in Diagostics/server/Workers . . . not before or after this PR. I'm not even sure if paging is supported here as it seems all of the workers are just displayed as rows with no paging.
  • Switching to Settings accordion shows paging before and shows form buttons after, so that is fixed.

@himdel are you certain it was showing a paging bar on Diagnostics/server/Workers prior to this fix?

@martinpovolny martinpovolny merged commit c047155 into ManageIQ:master Jan 18, 2018
@martinpovolny martinpovolny added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 18, 2018
@martinpovolny
Copy link
Member

@himdel : please, deal with the feedback in a follow-up PR if needed. Thx!

@himdel himdel deleted the paging-remove-bz1512957 branch January 18, 2018 10:44
@himdel
Copy link
Contributor Author

himdel commented Jan 18, 2018

@dclarizio Yes I am, but I also notice that there is still a fix needed .. if you go to Workers and then reload, the toolbar is there.

But if you go to Settings first, and then switch to Workers, it's not.

(paging_div is hidden in that case)

@himdel
Copy link
Contributor Author

himdel commented Jan 18, 2018

The problem where paging_div is present but hidden will be fixed in #3271

simaishi pushed a commit that referenced this pull request Jan 18, 2018
Remove paging (not paging_div) after leaving GTL screens
(cherry picked from commit c047155)

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

Gaprindashvili backport details:

$ git log -1
commit f7fc25e8935f072ace55374240e142dc7e164f42
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Thu Jan 18 10:57:27 2018 +0100

    Merge pull request #3262 from himdel/paging-remove-bz1512957
    
    Remove paging (not paging_div) after leaving GTL screens
    (cherry picked from commit c0471558f9ae23a22f5a4902dab9dffd864ca000)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536062

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.

6 participants