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

Clear Search box text #1893

Merged
merged 1 commit into from Sep 8, 2017
Merged

Conversation

GregP
Copy link
Contributor

@GregP GregP commented Aug 11, 2017

Added code to clear out Search box text, fixes 2 BZs. Keeps Search text while navigating through accordions on a page, but clears out Search text when button is clicked and renders unfiltered data display.

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

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

============================
Screen shots post code fix:

Unfiltered data display:
search clearance initial display vms and templates

Search text is cleared, unfiltered data rendered:
search clearance vms and templates after clear button click

Search text has no data match:
search clearance vms and templates filter not found

@miq-bot miq-bot added the wip label Aug 11, 2017
@GregP GregP force-pushed the search_box_text_clearance branch 2 times, most recently from c881eba to f8848e8 Compare August 17, 2017 20:29
if @explorer
# Reset both values when Search box Clear/Cancel is clicked
# For explorer pages only for now
@search_text = @sb[:search_text] = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

@GregP i think this should be outside of if @explorer check

@GregP
Copy link
Contributor Author

GregP commented Aug 18, 2017

@h-kataria Ready for review.

@GregP GregP changed the title [WIP] Clear Search box text Clear Search box text Aug 18, 2017
@miq-bot miq-bot removed the wip label Aug 18, 2017
@h-kataria h-kataria self-assigned this Aug 19, 2017
@@ -531,6 +531,13 @@ def filesystem_download
send_data fs.contents, :filename => fs.name
end

def adv_search_text_clear
if @_params[:controller] == "vm_infra"
Copy link
Contributor

Choose a reason for hiding this comment

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

@GregP do we need to only clear search text for Infrastructure VM explorer, wondering if it will just work for all explorers without this if condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is we do not yet set @explorer value to true when we process this clear text code. Timing issue at this point. But, yes, eventually, the preference is to provide app wide fix. Need more analysis before enabling all explorer pages.

@GregP GregP changed the title Clear Search box text [WIP] Clear Search box text Aug 22, 2017
@GregP
Copy link
Contributor Author

GregP commented Aug 22, 2017

@h-kataria Refactored code to pass along explorer. Back in [WIP] mode for now to see if this change works for application wide explorer pages.

@miq-bot miq-bot added the wip label Aug 22, 2017
@GregP GregP changed the title [WIP] Clear Search box text Clear Search box text Aug 23, 2017
@miq-bot miq-bot removed the wip label Aug 23, 2017
@miq-bot
Copy link
Member

miq-bot commented Aug 23, 2017

Checked commit GregP@8430b63 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@GregP
Copy link
Contributor Author

GregP commented Aug 23, 2017

@h-kataria Ready for review.

@h-kataria
Copy link
Contributor

verified in UI

@h-kataria h-kataria added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 8, 2017
@h-kataria h-kataria merged commit bbb3b95 into ManageIQ:master Sep 8, 2017
@simaishi
Copy link
Contributor

Euwe backport details:

$ git log -1
commit d947e964e3eaa13f16266b9828650e374a14be34
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Sep 8 13:54:53 2017 -0400

    Merge pull request #1893 from GregP/search_box_text_clearance
    
    Clear Search box text
    (cherry picked from commit bbb3b95786c3dffa5a233bd30b7572783406bc55)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1490434

@simaishi simaishi removed the euwe/yes label Sep 11, 2017
simaishi pushed a commit that referenced this pull request Nov 13, 2017
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit b52dff6f3d8a19f940065a2ab9f3e32b54054323
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Fri Sep 8 13:54:53 2017 -0400

    Merge pull request #1893 from GregP/search_box_text_clearance
    
    Clear Search box text
    (cherry picked from commit bbb3b95786c3dffa5a233bd30b7572783406bc55)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1496939

@martinpovolny
Copy link

@GregP, @h-kataria : I think that this PR breaks the clear search button in the Hosts.

@GregP
Copy link
Contributor Author

GregP commented Nov 29, 2017

@martinpovolny @h-kataria Per discussion, this fix was only intended for Explorer type pages. New code "funnels" all Search Text clearance user requests though a new central point, but only Explorer type pages then clear the search text box and reload page contents. All other pages, non Explorer type including Hosts page, act same way they used to prior to this fix: search text is cleared, but page contents not reloaded automatically. Hope this helps and resolves the issue/concern.

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.

None yet

6 participants