Problem/Motivation

The current heading spacings are inherited from Seven and doesn't match with the design system.

Proposed resolution

Update heading spacings to match with the designs.

Remaining tasks

User interface changes

CommentFileSizeAuthor
#8 interdiff.3083004.3-8.txt1.64 KBfhaeberle
#8 3083004-8.claro_.Update-heading-spacings.patch1.48 KBfhaeberle
#3 claro-heading-spacing-3083004-3.patch1.33 KBlauriii
#2 claro-heading-spacing-3083004-2.patch0 byteslauriii
Screen Shot 2019-09-23 at 15.25.38.png93.96 KBlauriii

Comments

lauriii created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Needs review
StatusFileSize
new claro-heading-spacing-3083004-2.patch0 bytes

Clicked through most pages to make sure the new spacings look fine. It looked fine on all pages that I tested.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new claro-heading-spacing-3083004-3.patch1.33 KB

Oops 🤪

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

I'm checking this...

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/css/src/base/elements.css
@@ -49,38 +49,38 @@ summary {
+  margin: var(--space-m) 0 var(--space-s);
...
+  margin: var(--space-m) 0 var(--space-s);
...
+  margin: var(--space-m) 0 var(--space-s);
...
+  margin: var(--space-m) 0 var(--space-s);
...
+  margin: var(--space-m) 0 var(--space-s);
...
+  margin: var(--space-m) 0 var(--space-s);

    I think it would be better to define only top and bottom margins here, so instead of margin: var(--space-m) 0 var(--space-s), define these with margin-top: var(--space-m) and margin-bottom: var(--space-s) 

  2. +++ b/css/src/base/elements.css
@@ -49,38 +49,38 @@ summary {
   font-size: 1.625em;
...
   font-size: 1.385em;
...
   font-size: 1.231em;
...
   font-size: 1.154em;
...
   font-size: 1.077em;
...
   font-size: 1.077em;

    Since we're here, it would be a good idea to update the font sizes as well imho, but since line-heights are undocumented (and are smaller than the font-size), maybe it's better to skip this step...

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

#5.1 May I ask why you prefer setting value only for top and bottom in this case?
#5.2 +1 to moving this to a follow-up. Let's ask the design team to define line-height for headings.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Issue tags: +Needs followup

Re #6:

#5.1: Even if it the two approach seem to be equal, I'm afraid of that defining side margins as 0 will lead to issues like #3047433: Check box input element not vey well aligned. If you are sure this will not happen, let's do that, but this approach is a cause for concern imho.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
Status: Needs work » Needs review
StatusFileSize
new 3083004-8.claro_.Update-heading-spacings.patch1.48 KB
new interdiff.3083004.3-8.txt1.64 KB

I added the suggested changes. @huzooka Yeah, it's a nit but can cause trouble.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

There's a difference between these two cases; checkbox is a component which has higher priority and will override the HTML elements default values as a result. I'm +1 to doing that when building components. However, here we are setting the default values for heading HTML elements so there shouldn't be anything with a lower weight, therefore it would be very unlikely that we would override anything as a result of specifying those margins. I still think we should go with #3.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented

I'm good with both approaches. If some code want to screw it up, it would do it anyway. Lauri made a fair point! Do we have existing "low level components" to compare (which approach was used)?

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Sorry, I missed these updates.

I'm okay with both #3 and #8, but I still prefer #8.

Pick one and commit it :)

ps we still need the follow-up.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: -Needs followup
Related issues: +#3083231: Update heading font-sizes and line-heights

Opened #3083231: Update heading font-sizes and line-heights

  • lauriii committed 772c4ad on 8.x-1.x 
    Issue #3083004 by lauriii, fhaeberle, huzooka: Update heading spacings
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed #3 given the arguments provided in #9.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.