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
825 stars 166 forks source link

Add our p-section--shallow spacing to h1,h2s by default #5260

Open lyubomir-popov opened 1 month ago

lyubomir-popov commented 1 month ago

Wrapping headings in p-section--shallow is one of the most comment QA comments I have to leave, which leads me to think there's room for improvement. My propoosal:

Before: image image

After: image image

syncronize-issues-to-jira[bot] commented 1 month ago

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/WD-13715.

This message was autogenerated

jmuzina commented 1 week ago

The quick way to do this is to extend the shallow section spacing placeholder in the heading 1 placeholder (which is extended by the heading 2 placeholder)

%vf-heading-1 {
    // same as %section-padding--shallow
    padding-bottom: $spv--x-large;
    // ..

However, this would be a breaking change for users who implement this spacing in the current way. They will end up with double the spacing (padding added to the section and to the header).

<div class="p-section--shallow">
  <h1>Some header text</h1>
   <!--Bottom padding here-->
</div>
<!--Bottom padding here-->

I suppose a workaround could be to create a padding collapse override for headings that are direct children of a shallow section, but I'm not sure how I feel about targeting a component class inside of a base style mixin.

%vf-heading-1 {
  // same as %section-padding--shallow
  padding-bottom: $spv--x-large;

  .p-section--shallow > & {
    padding-bottom: 0;
  }
}

@pastelcyborg @bartaz any thoughts?

Also @lyubomir-popov , should anything be done with the responsive spacing added to the headings? https://github.com/canonical/vanilla-framework/blob/327bc2e6cbb0801be76fb8feb3b53e416996f659/scss/_base_typography-definitions.scss#L25-L30

pastelcyborg commented 1 week ago

Possibly unpopular opinion: if the tools already exist in Vanilla to achieve the desired design, I would leave this as-is for now. Yes, we could probably very slightly optimize this, but I would counter with two sentiments:

  1. We often see design requesting modifications/changes via rounds of iteration; I think we should resist the urge to knee-jerk make sweeping code changes to accommodate what could be a fleeting request
  2. We have a lot of other large-scale work to complete, so if the current implementation and features aren't actually broken or prohibitive to completing sites, this should be a very low priority