feat(@vtmn/css): update price to allow no-padding

EGilleron commented 1 year ago

Changes description

Context

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

This PR is for completed this one : https://github.com/Decathlon/vitamin-web/pull/1341

To avoid some breaking changes, we've decided to create a new props called "noPadding" to remove the padding of the Price component.

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?

No

Other information

thibault-mahe commented 1 year ago

Hi @EGilleron, thanks for your suggestion !

Please take the time if you can to check the checkboxes and fill the template, it helps us for the reviews :)

I'm a bit mixed on this PR because I feel that we don't have the necessary tools (nested selectors, mixins, ...) on v0 to manage this kind of complexity (crossing behaviors between different variants)

We can see easily here that we are creating maintenance issues since now, for each new variant we would add, we will have to add a new class in all the size variants...

I think it would be more readable and understandable to either force a behavior on the variant we want, for instance :

.vtmn-price_variant--strikethrough {
  background-color: transparent;
  color: var(--vtmn-semantic-color_content-tertiary);
  text-decoration: line-through;
  // this variant never has a background and therefore never need padding
  min-block-size: 0 !important;
  block-size: auto !important;
}

or to isolate this behavior to explain it later:

// Some variants never has a background and therefore never need padding
.vtmn-price_variant--default,
.vtmn-price_variant--strikethrough {
  min-block-size: 0 !important;
  block-size: auto !important;
}

(Note that in each case the comment should be mandatory to explain our decision)

But even here it raises questions, especially "why can't a strikethrough variant have a background (and thus a padding) ?". In functional approach we are here clearly on a side effect: by adding the strikethrough variant, which logically brings a property that bars the price, we have as aside effect to remove the padding and forbid the background...

Maybe this behavior should be isolated, ~~either with a utility (and Tailwind offers one) or~~ with another variant that describes the behavior well:

.vtmn-price_variant--no-padding {
    padding: 0;
    min-block-size: 0;
    block-size: auto;
}

What do you think ?

EGilleron commented 1 year ago

Hi @thibault-mahe thanks for the feedback.

Ok i see where you want to go, i'm agree with the last part. So should add a new property "padding", not a new variant maybe you mean.

And this new property should set this new css property only with variants without background, like this :

/* Should apply no padding class only on variant without background */
.vtmn-price_variant--default.vtmn-price--no-padding,
.vtmn-price_variant--strikethrough.vtmn-price--no-padding{
  min-block-size: 0;
  block-size: auto;
  padding: 0;
}

because if it's only the class without "shield" the user will be able to apply no padding with variant with background....

And also, of course, rollback what have been done on this PR and the latest who was merged.

What do you think about that ?

thibault-mahe commented 1 year ago

I really meant a new variant, if we want to have a more functional (eg maintainable) code, so any user could create a mix of different modifiers (variant, size, ,padding).

Indeed, with this more maintainable approach, there would be less enforced constraint like you said.

If we want to have both a default enforced constraints (for some variants) and maintainable/readable code, we should fallback to the first solutions I suggested :

Either "local" specificity (1):

.vtmn-price_variant--strikethrough {
  background-color: transparent;
  color: var(--vtmn-semantic-color_content-tertiary);
  text-decoration: line-through;
  // this variant never has a background and therefore never need padding
  min-block-size: 0 !important;
  block-size: auto !important;
}

or an "isolated" approach (2):

// Some variants never has a background and therefore never need padding
.vtmn-price_variant--default,
.vtmn-price_variant--strikethrough {
  min-block-size: 0 !important;
  block-size: auto !important;
}

for my part I prefer approach (1 )

I'm still going to ask why we would forbid the strikethrough variants to have a padding

lauthieb commented 1 year ago

Hello, I convert this issue to draft because https://github.com/Decathlon/vitamin-design/issues/167 had been reopened & I also reverted #1341 (cf. my email). We will wait for this issue to be done, then we will develop in order to avoid breaking changes :) Thanks for your understanding!

lauthieb commented 1 year ago

Hi @EGilleron & @thibault-mahe,

FYI, the decision after discussing with @MARIEDELATTRE: https://github.com/Decathlon/vitamin-design/issues/167#issuecomment-1408181872

So, feel free to anticipate, we will have a new prop called "No padding" (boolean True/False). And, please, this PR will need to make this change for CSS, React, Svelte & Vue libraries.

Thanks a lot!

EGilleron commented 1 year ago

Hi @thibault-mahe and @lauthieb,

It is done on my side, I let you check, do not hesitate if you have any feedback, I am available:)

thibault-mahe commented 1 year ago

Seems ok to me, I merge it as-is. Thanks again for your contribution @EGilleron :)

Decathlon / vitamin-web