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

Topology - indicate no results when searching #1277

Merged
merged 5 commits into from
May 9, 2017
Merged

Topology - indicate no results when searching #1277

merged 5 commits into from
May 9, 2017

Conversation

himdel
Copy link
Contributor

@himdel himdel commented May 4, 2017

For some reason, when searching in topology, we want to inform the user when no results match the criteria.

We could just show an alert, but that takes up too much vertical space.
We could also hide the topology, but since topology still shows all the items, just with opacity=0.2, clearly it's supposed to be showing by design.

So.. a compromise, we show a patternfly-like message, but overlay it over the topology graph.

no-results

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


$scope.searchNode = function() {
var svg = topologyService.getSVG($scope.d3);
var query = $('input#search_topology')[0].value;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using angular.element here and even below?

angular.element('input#search_topology').value

var links = svg.selectAll("line");
links.style("opacity", "0.2");

if (nodes.length == selected.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks :)


$scope.searchNode = function() {
var svg = topologyService.getSVG($scope.d3);
var query = $('input#search_topology')[0].value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use angular.element here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not doing that, this has nothing to do with anything angular related, no point in using angular.element here.

(This looks out of place in an angular controller, I agree completely, but it is supposed to look out of place, because that's one of those things that's done in a silly way until we make the custom toolbar angular too.)

@miq-bot
Copy link
Member

miq-bot commented May 4, 2017

This pull request is not mergeable. Please rebase and repush.

@dclarizio
Copy link

@himdel can you verify that the Clear Search link works? :)

@AparnaKarve
Copy link
Contributor

@himdel I get the No results match the search criteria message even though the results match the search criteria. Check out the screenshot below.

screen shot 2017-05-05 at 2 25 44 pm

@AparnaKarve
Copy link
Contributor

Also, do we really need the Clear Search link? Seems redundant since we can clear the search in the Search box itself.

himdel added 4 commits May 9, 2017 11:47
…matches 0 items

included in all the topologies and styled so that it overlays the content

https://bugzilla.redhat.com/show_bug.cgi?id=1445857
…de instead of having N identical copies

this just moves searchNode and resetSearch to the service, under a `mixinSearch` method that adds these to the current scope when called
@himdel
Copy link
Contributor Author

himdel commented May 9, 2017

@dclarizio Verified Clear search works :) .. was it behaving oddly somewhere or..?

@AparnaKarve Well, it's kinda nice to have a straightforward way of cleaning the search .. but either way, it was part of the design proposed by patternfly, that's why.

@himdel himdel changed the title Topology - indicate no results when searching [WIP] Topology - indicate no results when searching May 9, 2017
@miq-bot miq-bot added the wip label May 9, 2017
because apparently length is always 0 or 1
@himdel himdel changed the title [WIP] Topology - indicate no results when searching Topology - indicate no results when searching May 9, 2017
@himdel
Copy link
Contributor Author

himdel commented May 9, 2017

@AparnaKarve thanks for catching the bug .. turns out with d3, length is not important, only size() matters :D

@himdel himdel removed the wip label May 9, 2017
@miq-bot
Copy link
Member

miq-bot commented May 9, 2017

Checked commits https://github.com/himdel/manageiq-ui-classic/compare/5a994811483601dec8cbb1c347789d04951f0d17~...203fddcded751e9567c638e83873424023ab758b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

LGTM

@skateman
Copy link
Member

skateman commented May 9, 2017

Tested in the UI, looks good!

@skateman
Copy link
Member

skateman commented May 9, 2017

Linking the refactoring pivotal story here as this PR unifies the search functionality in JS controllers: https://www.pivotaltracker.com/story/show/143239019

@dclarizio
Copy link

Travis failure is unrelated, merging.

@dclarizio dclarizio merged commit 9c1c972 into ManageIQ:master May 9, 2017
@dclarizio dclarizio added this to the Sprint 61 Ending May 22, 2017 milestone May 9, 2017
@himdel himdel deleted the topology-not-found branch May 9, 2017 17:41
simaishi pushed a commit that referenced this pull request Jun 8, 2017
Topology - indicate no results when searching
(cherry picked from commit 9c1c972)

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

simaishi commented Jun 8, 2017

Fine backport details:

$ git log -1
commit 87966ac97eb9de64e9675e4256bd0761395deaea
Author: Dan Clarizio <dclarizi@redhat.com>
Date:   Tue May 9 10:40:12 2017 -0700

    Merge pull request #1277 from himdel/topology-not-found
    
    Topology - indicate no results when searching
    (cherry picked from commit 9c1c9725cac763f57f91d8e5531b8c4818aac78d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459297

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