Skip to content

Add human readable labels to filter headings #3644

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

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Add human readable labels to filter headings #3644

merged 3 commits into from
Nov 17, 2023

Conversation

jpellizzari
Copy link
Contributor

@jpellizzari jpellizzari commented Nov 15, 2023

Closes #3590
Closes #3583

Adds human readable labels to the explorer filter, and removes facets that aren't relevant for a category.

Here is the Templates view; notice the "Template Type" label:
Screenshot from 2023-11-15 13-04-14

Here is the Applications view with the irrelevant facet removed:
Screenshot from 2023-11-15 13-04-28

@jpellizzari jpellizzari added the enhancement New feature or request label Nov 15, 2023
@jpellizzari jpellizzari force-pushed the 3590-labels branch 5 times, most recently from 2d8742b to 0f7e5d7 Compare November 15, 2023 21:01
@jpellizzari jpellizzari marked this pull request as ready for review November 15, 2023 21:27
Copy link
Contributor

@enekofb enekofb left a comment

Choose a reason for hiding this comment

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

Added some questions around design background / considerations. I believe mostly around labels mapping.

Relevant facets design looks good to me, however testing i could see that Explorer page gives me less filters than expected:

Screenshot 2023-11-16 at 15 11 21

I also tried to filter by template type without results but i expect one:

Screenshot 2023-11-16 at 15 19 31

Last point: to review extending explorer documentation in case of needs update.

@@ -277,6 +280,9 @@ var (
"weave.works/template-type",
},
Category: CategoryTemplate,
HumanReadableLabelKeys: map[string]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

please share your design considerations for adding it here vs other solutions like extending changing from Labels: []string{ to Labels: []label{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like overkill for a Label to be a first class object. I can see how it might be useful though. I will take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@enekofb Took a pass at this and it involves a lot of cross cutting changes, specifically to converting store.Facets from a generic map[string][]string into a proprietary struct. I think as it is in this PR it is a bit simpler.

Mind if we address in a follow up instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, i think that we could take this opportunity to kind of review our domain model and api ... let me setup something next week to pair on it

@jpellizzari jpellizzari requested a review from enekofb November 16, 2023 18:31
@jpellizzari
Copy link
Contributor Author

Added some questions around design background / considerations. I believe mostly around labels mapping.

@enekofb I cannot reproduce. Explorer is working as expected in my environment.

Screenshot from 2023-11-16 09-41-13

Maybe we can screenshare to figure it out.

Copy link
Contributor

@enekofb enekofb left a comment

Choose a reason for hiding this comment

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

Fantastic Jordan. Tested again and works now 🪂

Thanks for adding the tests.

@jpellizzari jpellizzari merged commit fd0e650 into main Nov 17, 2023
@jpellizzari jpellizzari deleted the 3590-labels branch November 17, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants