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.

Sign up for GitHub

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

Jump to bottom

Advanced search modal - only render once, always, hidden #5629

Merged
merged 6 commits into from
Jun 5, 2019
Merged

Advanced search modal - only render once, always, hidden #5629

merged 6 commits into from
Jun 5, 2019

Conversation

himdel
Copy link
Contributor

@himdel himdel commented May 27, 2019 •
edited
Loading

Currently the advanced search modal (layouts/_adv_search) is rendered twice on most screens:

  • from layouts/_footer, unconditionally called from application layout
  • from layouts/_x_adv_searchbox called from explorer controller explorer views
  • from layouts/_searchbar called from the center_div_with_listnav layout

And the inside of the modal contains logic asking for show_adv_search? and @edit[@expkey],
to deteremine whether to render the inside of the modal too.

But, all the ajax transition logic assumes the modal is always there, hidden, and tries to replace the inner divs' content (#adv_search_body, #adv_search_footer), failing when missing.

So.. this PR:

  • removes any advanced search modal rendering from anywhere but the footer
    • this does not affect rendering the trigger button, or a searchbox, only the modal
    • the modal gets rendered always, once
  • moves the "do I render the inner divs?" logic inside those divs, so that they always get rendered, but empty if no suitable content exists at the point
  • fixes all ajax x_adv_searchbox replaces to also replace the modal divs
  • fixes all ajax adv_search_body and adv_search_footer replaces to ignore the condition moved inside those templates when rendered standalone

Testing:

  • get to a vm detail screen
  • do a full screen reload
  • switch to the VMs accordion (or click the tree root if already there)
  • click Advanced Search

Before: the modal won't appear
After: the modal will appear

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

@miq-bot miq-bot changed the title Advanced search modal - only render once, always, hidden [WIP] Advanced search modal - only render once, always, hidden May 27, 2019
@miq-bot miq-bot added the wip label May 27, 2019
@himdel
Copy link
Contributor Author

himdel commented May 27, 2019 •
edited
Loading

TODO bunch of places replacing layouts/_x_adv_searchbox should probably be updating the modal too

and also make sure removing the "hidden if @edit && @edit[:adv_search_open] != true" logic can't break anythign - nothing is touching that div, and the top has its own display:none

@himdel
Copy link
Contributor Author

himdel commented May 28, 2019 •
edited
Loading

remaining TODO - the 2 places replacing just body and footer would previously ignore the show_adv_search? condition
now it's moved inside, so the body gets rendered empty because of it
added :force => true for those cases

@himdel
Copy link
Contributor Author

himdel commented May 29, 2019

@miq-bot remove_label wip

This could affect any advanced search - please don't only test vm infra :).

@miq-bot miq-bot changed the title [WIP] Advanced search modal - only render once, always, hidden Advanced search modal - only render once, always, hidden May 29, 2019
@miq-bot miq-bot removed the wip label May 29, 2019
@himdel
Copy link
Contributor Author

himdel commented May 30, 2019

cc @martinpovolny

@martinpovolny
Copy link
Member

@himdel : please rebase. I'll test and merge.

himdel added 2 commits May 31, 2019 14:56
@himdel
Don't render layouts/adv_search from anywhere but footer
348f1fb 
to prevent multiple copies of the modal from floating around,
the footer is always rendered in application layout
@himdel
adv_search - move show_adv_search? logic inside adv_search_{body,foot…
37f7d5f 
…er} divs

we replace those divs' content when doing ajax transitions, so they have to be present unconditionally
they don't have to have anything inside though

Also, a bunch of url_for calls need to only happen under that condition, so moved too.
(Not all controllers have a `adv_search_load_choice` route for example.)

(also fixes random missing gettext around Close)

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

himdel commented May 31, 2019

Rebased, thanks :)

himdel added 4 commits May 31, 2019 15:03
@himdel
replace_search_box - unify
79bf406 
always use this function to replace adv_searchbox_div
overriden in some controllers for extra gtl bits
moved the nameonly params to the function caller

no functional changes yet
@himdel
replace_search_box - also replace the modal content
a082d8e 
now that the adv_search partial is not included from x_adv_searchbox, we need to replace it separately, each time searchbox would have been replaced
@himdel
adv_search_{body,footer} - allow ignoring the show_adv_search conditi…
e7965d2 
…on when needed

the body and footer now have a conditional inside the main div, moved from the _adv_search partial,
to make sure the divs are always rendered

but all the places rendering the body and footer directly (via ajax) didn't previously need the condition to be true,
and it isn't

so, adding a force=true param to any place which just replaces the body and footer of advanced search, without goingh through the top partial
@himdel
vm_common spec - update render expects to match replace_search_box ch…
3800fd9 
…anges

we also need to expect an layouts/adv_search render, now that the modal is not part of x_adv_searchbox
@miq-bot
Copy link
Member

miq-bot commented May 31, 2019

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/348f1fb78d3bb24f73785ad606312a5216020939~...3800fd9f4871980fad86882139a39ea66c8b1ada with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
11 files checked, 0 offenses detected
Everything looks fine. 🍪

@martinpovolny martinpovolny self-assigned this Jun 5, 2019
@martinpovolny
Copy link
Member

  • I tested several other pages. All work.
  • The code looks better with the extracted method.
  • The new organization makes more sense and is actually simpler.

Good job!

@martinpovolny martinpovolny merged commit f8d7131 into ManageIQ:master Jun 5, 2019
@martinpovolny martinpovolny added this to the Sprint 113 Ending Jun 10, 2019 milestone Jun 5, 2019
simaishi pushed a commit that referenced this pull request Jun 10, 2019
@martinpovolny @simaishi
Merge pull request #5629 from himdel/adv-search
a780d21 
Advanced search modal - only render once, always, hidden

(cherry picked from commit f8d7131)

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

Hammer backport details:

$ git log -1
commit a780d212d4ccfd8d4864d301669260859f5bd8eb
Author: Martin Povolny <[email protected]>
Date:   Wed Jun 5 20:04:05 2019 +0200

    Merge pull request #5629 from himdel/adv-search
    
    Advanced search modal - only render once, always, hidden
    
    (cherry picked from commit f8d713149e3ef6a46b21c9eb51a084c606a984a3)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1717923

@himdel himdel deleted the adv-search branch June 10, 2019 13:57
@h-kataria h-kataria mentioned this pull request Jun 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@martinpovolny martinpovolny

Labels
blocker bug hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@himdel @martinpovolny @miq-bot @simaishi