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

feat(admin-ui): improve facet selector with the code #3175

Merged

Conversation

alexisvigoureux
Copy link
Contributor

@alexisvigoureux alexisvigoureux commented Oct 31, 2024

Description

Sometimes multiple facets have the same name so it's impossible to distinguish them except with code name
This PR introduces the code name in the facet value chip component

PS: I have not found the way to generate the graphql schema for the admin-ui directory

Breaking changes

No

Test

Add a facet with the same name (but different code) as another one and with the same facetValues
Go to http://localhost:4200/admin/catalog/collections/2
Add the facet in the collection filters

Screenshots

Add a popover to include more context about the facet type
Capture d’écran 2024-10-31 à 19 16 49

Update the title displayed on hover to include more context about the facet type
Capture d’écran 2024-10-31 à 19 17 06

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

vercel bot commented Oct 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Nov 12, 2024 9:26am

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement! I've left a couple of comments I'd like you to look at.

@@ -13,4 +13,9 @@ export class FacetValueChipComponent {
@Input() removable = true;
@Input() displayFacetName = true;
@Output() remove = new EventEmitter<void>();

get formattedTitle(): string {
const facetCode = this.facetValue.facet.code ? `(${this.facetValue.facet.code}) ` : '';
Copy link
Member

Choose a reason for hiding this comment

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

What's the need for this line? Do you expect that the facet.code may sometimes be falsy?

Copy link
Contributor Author

@alexisvigoureux alexisvigoureux Nov 12, 2024

Choose a reason for hiding this comment

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

I expect the code to be empty (I've updated the code to reflect this) if it's not requested in the graphql request
b499e03 (example: the product page)

@michaelbromley michaelbromley merged commit 35892a5 into vendure-ecommerce:minor Nov 15, 2024
20 of 30 checks passed
@michaelbromley
Copy link
Member

Thank you!

@github-actions github-actions bot locked and limited conversation to collaborators Nov 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants