canonical / vanilla-framework

From community websites to web applications, this CSS framework will help you achieve a consistent look and feel.
https://vanillaframework.io
GNU Lesser General Public License v3.0
850 stars 169 forks source link

Make separator styling consistent #5338

Closed bartaz closed 2 months ago

bartaz commented 2 months ago

Context

This partially reverts changes done in #5241.

The state of the separators that this PR proposes is as follows:

Rule p-rule component is not deprecated, it aims to be the default and main way to create separators. Base <hr> element has default separator styling, same as .p-rule component. Both are horizontal lines with bottom margin.

Any additional styling options (muted, highlighted, accented) are provided by rule component. The "variants" of hr element are deprecated.

So ideally everything should be covered by rule component:

Open questions

  1. Should we keep both "Separators" and "Rule" component documentation pages? If we decide to merge them into one, where should it live?

As discussed, the base "Separators" page will be removed. All options (including mention of deprecated styles) are going to be covered by Rule component page.

  1. How useful is the hr.is-fixed-width styling. Currently it allows to limit the width of hr to the size of grid without wrapping it in grid. But it's inconsistent with rest of the framework and technically we should avoid doing it on "base" element. a) drop it, and expect devs to wrap separators inside the grid properly (adds a bit of markup) b) move it to p-rule component, but deprecate on base "hr" (keeps the variants of base elements, more consistent) c) keep as is?

For now we are keeping the is-fixed-width class name, as devs find it useful. We are officially moving it into rule component (and deprecating on base hr element).

Done

To achieve the status mentioned above, in this PR:

QA

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

webteam-app commented 2 months ago

Demo

Jenkins

demos.haus

jmuzina commented 2 months ago

Discussed in pair programming meeting today:

Should we keep both "Separators" and "Rule" component documentation pages? If we decide to merge them into one, where should it live?

I think "Separators" page can be removed, in favor of just using "Rule" page. This PR seems like it makes HR behave much like the default button which is documented as a component, despite it not needing p-button class

How useful is the hr.is-fixed-width styling. Currently it allows to limit the width of hr to the size of grid without wrapping it in grid. But it's inconsistent with rest of the framework and technically we should avoid doing it on "base" element. a) drop it, and expect devs to wrap separators inside the grid properly (adds a bit of markup) b) move it to p-rule component, but deprecate on base "hr" (keeps the variants of base elements, more consistent) c) keep as is?

@pastelcyborg You made this point more succinctly, but I'll summarize my understanding: I think the general direction we're going for is to keep grid behavior under the grid page & mixin, and expect people to add markup to support it. Though that would be a breaking change, so maybe it should be set for next major version.

immortalcodes commented 2 months ago

I have applied p-rule--muted class to <hr> within a section, in this case it becomes very close to the button, will this be considered fine? image

bartaz commented 2 months ago

I have applied p-rule--muted class to <hr> within a section, in this case it becomes very close to the button, will this be considered fine?

@immortalcodes In current version of Vanilla that's what happens. But this PR fixes it by adding margin to p-rule (consistent with plain <hr>). So once this PR is merged and released this will be a correct way to do it.

mtruj013 commented 2 months ago

re: the open questions:

  1. IMO it there should only be one documentation page for hrs. Since p-rule is the base component for quite a lot of variants I think that it should be the one that stays
  2. I find hr.is-fixed-width to be pretty useful and my vote would be for option B, move it to the rule component