feat(@vtmn/css): improve `VtmnPrice` component by removing padding

Decathlon / vitamin-web

Decathlon Design System UI components for web applications

https://decathlon.github.io/vitamin-web

Apache License 2.0

282 stars 76 forks source link

feat(@vtmn/css): improve `VtmnPrice` component by removing padding #1341

Closed EGilleron closed 1 year ago

EGilleron commented 1 year ago

…ikethrough variants

Changes description & Context

Issue here https://github.com/Decathlon/vitamin-design/issues/167

Checklist

[x] Make sure you are requesting to pull a topic/feature/bugfix branch. Please, don't request directly from your main!
[x] Check commits & PR names matches our requested structure. It must follow the https://www.conventionalcommits.org pattern.
[x] Check your code additions will fail neither code linting checks.
[x] I have reviewed the submitted code.
[x] I have tested on related showcases.
[x] If it includes design changes, please ask for a review design-system-core-team-design GitHub team.

Does this introduce a breaking change?

is removing the padding-inline for default and strikethrough vartiant of VtmnPrice so is a breaking change

Tlahey commented 1 year ago

Thanks @EGilleron for the PR :)

I have a question about the padding-inline. I think (for me), it's better to remove this value from the .vtmn-price and set padding-inline: var(--vtmn-spacing_2); only for vtmn-price_variant--accent and vtmn-price_variant--alert.

Because by default, the component should not have padding-line. It's because we integrate a background color that we want to put an inline.

thibault-mahe commented 1 year ago

Hi @EGilleron thanks for your PR ! I agree with @Tlahey on settings properties only where they make sense instead of overriding them :)

EGilleron commented 1 year ago

Hi @EGilleron thanks for your PR ! I agree with @Tlahey on settings properties only where they make sense instead of overriding them :)

I agree with you and @Tlahey makes sense, it was fixed with a new commit 🤟🏻

thibault-mahe commented 1 year ago

Perfect for me, I merge this as-is. Thanks again for your contribution @EGilleron , really appreciated :)

lauthieb commented 1 year ago

Hello, I had reverted this PR because https://github.com/Decathlon/vitamin-design/issues/167 had been reopened & I also converted to draft #1343 (cf. my email). Thanks for your understanding!