feat(vwc-icon-button): viv-306 layout, shape, connotation support #504
Conversation
|
PR showcase deployed here. |
|
|
||
| @include shape-mixins.shape( | ||
| $shapes: ( | ||
| rounded: 6px, |
There was a problem hiding this comment.
rounded is 6px by default. can we exclude this from override somehow?
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
|
PR showcase deployed here. |
| @@ -3,14 +3,15 @@ | |||
| @use '../functions'; | |||
|
|
|||
| // override to apply relevance | |||
| $layouts: filled outlined soft text !default; | |||
| $layouts: filled outlined soft text ghost !default; | |||
There was a problem hiding this comment.
what's the difference between text & ghost?
There was a problem hiding this comment.
text and ghost styles are the same.
I didn't think layout="text" was appropriate for icon-button as there is no text in this element.
What do you think?
There was a problem hiding this comment.
Is anyone using layout="text" buttons?
There was a problem hiding this comment.
I'll check, if so, we can keep both as css selectors for backwards compatibility
There was a problem hiding this comment.
maybe late to the party, but:
- IMHO definitely just one of those should stay, and since it is used in
vwc-icon-buttonwhich is text-less, as @JoelGraham93 mentioned,ghostsounds better - maybe
transparentis even better thanghost?
There was a problem hiding this comment.
hmmm, if had to pick, I would 'ghost'. but am up for more ideas on that
There was a problem hiding this comment.
I'm happy with ghost as it's a common term for buttons with no fill colour - though they usually have a border.
Was there any discoveries about removing layout="text"? I see PR is labeled ready-to-merge
|
|
||
| :host([layout='outlined']) { | ||
| .mdc-icon-button:disabled { | ||
| border-color: currentColor; |
There was a problem hiding this comment.
we previously had discussed this value as candidate for foreground color and decided to explicitly define the foreground variable. I dont mind either way but I'd vote for consistency
There was a problem hiding this comment.
assigned var in 544f26a
| options: Object.values(Shape) | ||
| options: Object.values(Shape).filter(s => [ | ||
| Shape.Rounded, Shape.Pill | ||
| ].includes(s)), |
There was a problem hiding this comment.
why not just: [Shape.Rounded, Shape.Pill]?
There was a problem hiding this comment.
dunno, it's just more supervised. it follows convention anyway, which is what we encourage, so better if this PR merges as is, and if we change this convention we do it in a dedicated PR
| options: Object.values(Shape), | ||
| options: Object.values(Shape).filter(s => [ | ||
| Shape.Rounded, Shape.Pill | ||
| ].includes(s)), |
There was a problem hiding this comment.
same as remark above - why not simple array of white listed values?
| Connotation.Info, | ||
| Connotation.Announcement, | ||
| ].includes(c) | ||
| ); |
There was a problem hiding this comment.
same as above - why not a simple array?
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 330b36c |
|
Kudos, SonarCloud Quality Gate passed!
|
This PR can make use of foundation
layoutsconst andhandleMultipleSizePropsutil in #485