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

Layouts - always create paging_div, hidden if needed #3271

Merged
merged 4 commits into from
Jan 19, 2018
Merged

Layouts - always create paging_div, hidden if needed #3271

merged 4 commits into from
Jan 19, 2018

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Jan 18, 2018

content layout - always creates paging_div, after #main-content
center div with/no listnav - create paging_div conditionally, after #main-content
center div dashboard - no paging_div, but no ajax transitions to other screens either

this updates center div with/without listnav to always create paging_div, hidden if necessary

that way, ReportDataController.prototype.movePagination should always succeed, and the pagination will never appear above GTL.

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

(also https://bugzilla.redhat.com/show_bug.cgi?id=1535946 which is marked as a dup)

@h-kataria
Copy link
Contributor

@karelhala @martinpovolny please review.

@himdel
Copy link
Contributor Author

himdel commented Jan 18, 2018

Not yet please .. this is still missing the bit where we show the div when hidden, but only in cases where we don't set @pages to nil.

@himdel
Copy link
Contributor Author

himdel commented Jan 18, 2018

Testing:

  1. Add some infrastructure provider.
  2. Navigate to Compute/Infrastructure/Datastores.
  3. Open details screen of some datastore.
  4. Click on "Managed/Registered VMs" and "Managed/Unregistered VMs" fields in Relationship table

For me, the URL is /storage/show/10000000000026?display=registered_vms.

The page is using center_div_no_listnav, layout_uses_paging? returns false, @in_a_form is nil => paging should not be visible in that screen.

EDIT: except that @pages is set, so it will be visible unless we explicitly (@pages = nil)

content layout - always creates paging_div, under #main-content
center div with/no listnav - create paging_div conditionally, *after* #main-content
center div dashboard - no paging_div, but no ajax transitions to other screens either

this updates center div with/without listnav to always create paging_div, hidden if necessary

that way, ReportDataController.prototype.movePagination should always succeed, and the pagination will never appear above GTL.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1501035
those classes don't do anything but make it easier to determine the current layout
@himdel
Copy link
Contributor Author

himdel commented Jan 18, 2018

Testing 2:

  1. (clean session, fresh after login)
  2. Go to Ops
  3. Switch to Diagnostics accordion
  4. Switch to Workers tab in server detail

before/after: no paging visible

  1. Reload the screen

before: paging visible
after: no paging visible

TODO: ^this is not true yet

EDIT: this is true as well

… truthy

assuming @pages being truthy means we should be showing paging,
this sends the paging data to GTL,
and makes the GTL to show paging_div when present.
this should work for miq-pagination because the GTL will unhide it

OTOH if there are forms which expect paging_div to be visible without explicitly unhiding it, they will be missing their buttons
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.

Codewise looks good and simple. Will test on couple of screens if it won't break anything.

@himdel
Copy link
Contributor Author

himdel commented Jan 19, 2018

This should be ready for testing...

I'm pretty sure this should mean no more paging above gtl.

I'm almost sure paging should always be there when it should.

I'm a bit worried that form buttons may get hidden on forms which don't explicitly show paging_div.

@himdel himdel removed the wip label Jan 19, 2018
@himdel himdel changed the title [WIP] Layouts - always create paging_div, hidden if needed Layouts - always create paging_div, hidden if needed Jan 19, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 19, 2018

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/65b5d1c6c25d1ef59126e3a2f1cebb6c4dbf36d5~...989dc324bc6cc3ab93180350485d57bd37b01aa4 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.5-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@ZitaNemeckova
Copy link
Contributor

ZitaNemeckova commented Jan 19, 2018

Tested in UI. LGTM 👍

@karelhala
Copy link
Contributor

Tested on multiple edit/config/policy screens and everything looks good.

@martinpovolny martinpovolny merged commit b0e99b6 into ManageIQ:master Jan 19, 2018
@martinpovolny martinpovolny modified the milestones: bug, Sprint 78 Ending Jan 29, 2018 Jan 19, 2018
@himdel himdel deleted the paging-show-bz1501035 branch January 19, 2018 15:39
simaishi pushed a commit that referenced this pull request Jan 19, 2018
Layouts - always create paging_div, hidden if needed
(cherry picked from commit b0e99b6)

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

Gaprindashvili backport details:

$ git log -1
commit 1ce45d66c70c44b10233933ccad8953114c74d71
Author: Martin Povolny <[email protected]>
Date:   Fri Jan 19 16:24:27 2018 +0100

    Merge pull request #3271 from himdel/paging-show-bz1501035
    
    Layouts - always create paging_div, hidden if needed
    (cherry picked from commit b0e99b6b084e8aa5289e192e55728e42fd926159)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1536563

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.

7 participants