Feature/add components count on item cards#39
Merged
Conversation
added 7 commits
July 3, 2026 12:20
jordivx
reviewed
Jul 3, 2026
| if (hasComponentCount) { | ||
| this.filteredProducts = [...this.filteredProducts.map(product => ({ | ||
| ...product, | ||
| tags: hasComponentCount |
Contributor
There was a problem hiding this comment.
You are inside an if for when it has componentCount, so the ternary here is not needed, no?
| appshell-product-view-screen.owner-view appshell-chip:first-child mat-chip.appshell-chip { | ||
| --mdc-chip-elevated-container-color: var(--appshell-color-grey-warm) !important; | ||
| --mdc-chip-elevated-disabled-container-color: var(--appshell-color-grey-warm) !important; | ||
| --mdc-chip-label-text-color: var(--appshell-color-grey-warm-contrast) !important; |
Contributor
There was a problem hiding this comment.
According to the figma, the text color should be "--appshell-color-grey-strong"
| --mdc-dialog-container-shape: 0px; | ||
| } | ||
|
|
||
| appshell-product-catalog-screen.owner-view appshell-chip:first-of-type mat-chip.appshell-chip, |
Contributor
There was a problem hiding this comment.
I have a couple of thoughts about this, not required to be changed, just questions to consider (can have a no for answer):
- Why adding it in the global styles instead of in each component's styles (with ViewEncapsulation.None). If placed there it is isolated in the component and easy to understand where the style comes from.
- If staying here, could be simplified to a single selector by removing the first parts and starting from .owner-view ... ? Despite working I have doubts which approach would be better.
Contributor
Author
There was a problem hiding this comment.
No problem, suggestions and questions are more than welcome, and I'm always open for discussions 😊:
- I've put the styles at a global scope since we're using the appshell as an extermal library, not styling our own components. It's like a convention.
- It could be simplified, but I chose to make it more explicit just to make sure that, if in any future the owner-view css class is used somewhere else, this styling won't affect that new usage. This is the point of writing all the selectors, we're targeting only those chip cards.
|
jordivx
approved these changes
Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.