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: Search/filtering 'Filtered by:' search status #9442

Merged
merged 13 commits into from
Jan 25, 2021

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Dec 21, 2020

This PR adds a 'status' for the filtering/searching in the UI, without this its not super clear that you are filtering a recordset due to the menu selections being hidden once closed:

filter-by

This is the first area where we begin to use the i18n t helper, in this case to translate machine to human (en-US), which from a code perspective is probably the biggest change here. We've tried to get a good balance between providing some sort of nice fallback for non-translated text and sharing as many terms, words and phrases as possible.

Notes:

  1. We cleaned up a tonne of variable names where we had confusing "should this name be singular or plural' patterns, most things here should now use singular naming patterns.
  2. We reworked part of out PopoverSelect to convert OptGroup and Option to use glimmer and simplify things here, we also added a required property to specify that the menu should have at least 1 option selected.
  3. Translation keys use t with its default fallbacks, if no key is found it will dash-split and titlecase the final part of the key so something like common.brand.unknown-brand will end up as Unknown brand if the translation doesn't exist.

This PR only contains this feature for the main Service listing, but will now be rolled out across the rest of the UI.

Internationalisation of text will be rolled out gradually as and when until we have a fully i18n'able UI, but moving forwards any new features should make use of ember-intl from the start.

Edit:

I decided to make this PR the base branch for integrating across the rest of the ui:

@@ -1 +1,48 @@
common:
brand:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to have a chat about how we want to organise this before it gets used a lot.

In using it I figured these keys would be good keys to have, so common, common.brand, components.* to mirror our component tree etc. I had a few thoughts on some others like whether we should have a model.* key also to mirror our models for naming model key names etc. and one for routes.* again mirror the route tree.

I think I'd like to split this into files also, but I guess that can be decided on/done a little later.

Lastly, one thing I would really like to do is have every app/components/component-name/ folder contain its own translation.yaml file along with the hbs, js, css etc. I definitely like to look at doing this before we get too far into it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't remember if we discussed this. I do need to use a translation file atm for auth methods. Let's chat and get on the same page about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, ping me when you are around and we can decide how we want to do it

Copy link
Contributor

@kaxcode kaxcode left a comment

Choose a reason for hiding this comment

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

LGTM

@johncowen
Copy link
Contributor Author

Thanks for the review! I decided to make this PR the base branch for integrating across the rest of the ui, so I'll merge some other PRs down onto this before merging it all together onto master, so I added some TODO in the PR description.

@johncowen johncowen changed the title ui: Search/filtering 'Filtered by:" search status ui: Search/filtering 'Filtered by:' search status Jan 5, 2021
@vercel
Copy link

vercel bot commented Jan 19, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

consul-ui-staging – ./ui

🔍 Inspect: https://vercel.com/hashicorp/consul-ui-staging/6s3pef2n1
✅ Preview: Failed

[Deployment for 9524c7d canceled]

johncowen and others added 9 commits January 21, 2021 11:08
1. Reworks popover-select option and optgroup to glimmer components
2. Reworks to use the components themselves to tracks state rather than
a separate Set
3. Adds `required` for required at least one option to be selected and
removes substractive, which we no longer need
1. This contains the 'Filter by' functionality that we are going to use
on all search-bars
Also, make all filter names singular instead of plural
Previously when removing a special 'searchproperty' filter, it would
remove all the search property filters instead of only the one you had
clicked.

This fixes that up, adds tests and adds copious amounts of documentation
around the filter data reshaping function for future self/somebody
…9622)

* Filter by for healthchecks search-bar

* Add filter by for service instances

* Service Instances needs to check brands

* Update Service > Instances search-bar

* Lowercase the check types
* Add filtered by to Upstreams and Linked Services

* Filterby for upstream instances

* More specific headers for definition tables

* Add empty healthcheck filter predicate and related computed props

* More i18n
@vercel vercel bot temporarily deployed to Preview – consul January 25, 2021 18:12 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 25, 2021 18:12 Inactive
@johncowen johncowen merged commit bb95738 into master Jan 25, 2021
@johncowen johncowen deleted the ui/feature/services-filter-by branch January 25, 2021 18:13
@hashicorp-ci
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/316285.

@hashicorp-ci
Copy link
Contributor

🍒✅ Cherry pick of commit bb95738 onto release/1.9.x succeeded!

hashicorp-ci pushed a commit that referenced this pull request Jan 25, 2021
Adds a 'status' for the filtering/searching in the UI, without this its not super clear that you are filtering a recordset due to the menu selections being hidden once closed. You can also use the pills in this status view to delete individual filters.
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.

5 participants