Variables for spacing values used for spacing mixins

[ichim-david]

No description provided.

[sneridagh]

@danalvrz @steffenri Can I have also a review from you both? thanks!

[steffenri]

This also makes no sense. the figcaption margins are not controlling space between blocks but inside the block itself. Please keep the numeric values here.

[ichim-david]

@steffenri I disagree that the figcaption is controlling space within itself. that would be a job for padding.
The margin is used to give some spacing from the section to the element above it.

To me this is clear that it's a job for a generic vertical-space unit that isn't called block therefore
triggering the idea that it is placed on wrong elements given the fact that there are several
mixins and variables that use the naming vertical-space or spacing

[sneridagh]

@ichim-david The value in there is not generic, nor tied to the generic vertical spacing. It's tied to the figcaption, and it has intrinsic properties, defining the figcaption and how it behaves with its soroundings. If I change the generic one, I most probably don't want to change this.

[steffenri]

@sneridagh @ichim-david I added a few comments. Otherwise it looks good to me. We have to be REALLY careful with what we use variables for and what not. The key is to use variables for things that belong together when we change the variable. For example when we change the block spacing variable, it should change the block spacing on every block but nothing else.

[ichim-david]

@sneridagh @ichim-david I added a few comments. Otherwise it looks good to me. We have to be REALLY careful with what we use variables for and what not. The key is to use variables for things that belong together when we change the variable. For example when we change the block spacing variable, it should change the block spacing on every block but nothing else.

@steffenri thank you for your review, I understand your concern for having a variable named $block-vertical-space used on content types.
Nevertheless, these content types have the same margin that is usually assigned as the $block-vertical-space and it concerns a top or bottom margin value that is used as vertical spacing.

Questions:

1. What variable name would you think is fitting then for these values?
2. Is a generic variable good to be used on all of these places where you've signaled that it deals with content type?
3. Do you want a separate variable for each instance?

I assume no, and I am curious about your thinking, if you were to be given volto-light-theme to make a website how much flexibility in values would you like to have and what sections do you think it's normal to be static values that you assume you simply need to write CSS overrides.

@sneridagh @steffenri If you think and you give me a set of guidelines on what is acceptable to be pluggable in your styling I will make sure that my pull requests will be on those aspects and not something else.

[sneridagh]

I think @steffenri has a point in here.
Let's say that I change the $block-vertical-space and I don't want to change the figcaption vertical space...
It would get changed, maybe I don't want that. This enters the classic "early optimization" pitfall. We should not add a variable if we don't even know if it will be useful in the future.

Regarding directives to use, I'd say we can't stablish one... also, we don't have the development power not the vision of a theming library like bootstrap or others. So I'd say that we should go with the flow, improve on the go, don't go for early optimizations actions if they do not make sense today, because it could bite us in the future.

@ichim-david using common vars are ok, but there is a compromise if things are not really generic.

Also, let's assume that when CSS applies, everybody has its own way and do things following what's on your own book. There's nothing in IT more opinionated as doing CSS. And, as I said before, any of us are not Mark Otto... and the community does not have such a profile. So let's assume this fact and go with the best we have. At the end, adding a var or not, or using a declaration or another, is only one CSS override away in the cascade...

[ichim-david]

I think @steffenri has a point in here. Let's say that I change the $block-vertical-space and I don't want to change the figcaption vertical space... It would get changed, maybe I don't want that. This enters the classic "early optimization" pitfall. We should not add a variable if we don't even know if it will be useful in the future.

Regarding directives to use, I'd say we can't stablish one... also, we don't have the development power not the vision of a theming library like bootstrap or others. So I'd say that we should go with the flow, improve on the go, don't go for early optimizations actions if they do not make sense today, because it could bite us in the future.

@ichim-david using common vars are ok, but there is a compromise if things are not really generic.

Also, let's assume that when CSS applies, everybody has its own way and do things following what's on your own book. There's nothing in IT more opinionated as doing CSS. And, as I said before, any of us are not Mark Otto... and the community does not have such a profile. So let's assume this fact and go with the best we have. At the end, adding a var or not, or using a declaration or another, is only one CSS override away in the cascade...

I will see if I can find any similarity between the content as to why the value for the block-vertical-space is used.
If I find it I will add a suggestion for another name then that ensures that if we change the block-vertical-scape you can also change this space for these rules.
If I can't find it I will restore the static values and I will leave only use of the spacing variables for the mixins

[ichim-david]

@sneridagh @steffenri I have removed any extra changes I've made in order to keep this pull request pure sticking to the title of the pull request.
I believe this makes it an easy merge since nothing changes expect for using the spacing values in the mixins instead of static values.

I believe we need a vertical-space variable that uses the 25px that can be used just like the $block-vertical-space where you need to have this 25px value.
If we look at this CSS rule added by Victor https://github.com/kitconcept/volto-light-theme/blob/main/src/theme/_typo-custom.scss#L43 the block-vertical-space is used on p,ul,o where you could argue with the same logic that it doesn't concern blocks although it does concern block elements.

This is why I was asking you @steffenri for a naming suggestion so that we can apply it where we deal with vertical spacing margins.

If you have a name you desire I volunteer to add another pull request and ensure the 25px margins are using that variable.

[sneridagh]

I believe we need a vertical-space variable that uses the 25px that can be used just like the $block-vertical-space where you need to have this 25px value. If we look at this CSS rule added by Victor https://github.com/kitconcept/volto-light-theme/blob/main/src/theme/_typo-custom.scss#L43 the block-vertical-space is used on p,ul,o where you could argue with the same logic that it doesn't concern blocks although it does concern block elements.

That rule has a reasoning. p, o and ul are blocks translated to the block system. A figcaption does not, it's an internal element inside a block, thus, it does not have to abide with blocks rules.

[sneridagh]

@steffenri could you give it another try?

[steffenri]

@sneridagh @ichim-david This looks good now. Can be merged IMO.

[danalvrz]

Looks good! thanks @ichim-david!