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

UI - Tag Text Search #4383

Merged
merged 4 commits into from
Jul 18, 2018
Merged

UI - Tag Text Search #4383

merged 4 commits into from
Jul 18, 2018

Conversation

johncowen
Copy link
Contributor

This PR adds simple text based searching by tag.

You can now enter a tag name in the service listing pages/tabs and the result will include services with that tag.

This is a temporary text search for tags before looking into something more complex.

I've split the tests off into the first commit so if you wan to go back a commit to see it breaking you can.

@johncowen johncowen requested a review from a team July 12, 2018 13:53
@@ -23,6 +23,10 @@ export default Controller.extend(WithFiltering, {
get(item, 'Service')
.toLowerCase()
.indexOf(term) !== -1 ||
(get(item, 'Tags') || [])
.join('')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably an edge case, but would it be weird if the match here is where two tags get elided? Would it be better to join on space instead of empty string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm yeah, so I suppose with tags like 'httpd, db' a search for 'pddb' would hit, will turn this into a some or something 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@meirish went for a some for the moment at least, gonna merge but feel free to yell if something looks off with this, I added a couple more tests also, so should be good.

Copy link
Collaborator

@meirish meirish left a comment

Choose a reason for hiding this comment

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

👍 Looks good! Commented on a possible edge case, but I do think it's very edge case-y.

@johncowen johncowen merged commit d4b548a into master Jul 18, 2018
@johncowen johncowen deleted the feature/ui-tag-text-search branch July 18, 2018 17:38
@OneCricketeer
Copy link

While functional, it seems to flood the browser history by updating the url. Is that expected?

image

@OneCricketeer
Copy link

Might only be when there is a status that it happens.
image

@johncowen
Copy link
Contributor Author

Hi @Cricket007

I think there is a case to be said for either way for this (add to history/don't add to history). I actually fall down on the same side as you for this, I don't think typing into the search field should add to the history, but I'm guessing there are opinions that fall on the other side also. Happy for you to file an issue for this so we can discuss and track it, or I can do it for you? Would be nicer to get your user against a new issue though. Up to you, let me know.

Thanks,

John

@OneCricketeer
Copy link

@johncowen I would be interested in hearing why that feature could be wanted. #4614

@johncowen johncowen added the theme/ui Anything related to the UI label Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants