Heading component and fluid size improvements #679

tylersticka · 2020-05-01T22:00:42Z

Overview

This PR started as a resolution to #548 but I immediately noticed some heading issues. Solving those sparked some ideas on how to effectively port our Hash Heading pattern, and before I knew it I had a whole new component.

Fixes an issue in the headings.level mixin where em units were used instead of rem, causing fluid sizes to become out of step when font-size differed from 1em.
Simplifies headings.level based on actual heading usage.
Adds support for non-standard levels (beyond 1–6) in headings.level mixin.
Adds c-heading component.
Adds c-heading--level-* modifiers for changing the visual level.
Adds permalink variation of c-heading and necessary Twig logic for that. Permalink supports dark theme and has unique focus styles.
Adds ms.step-class-segment mixin for keeping translation of negative steps to n* steps DRY.
Adds unit.swap function and normalizes a few other functions in that file.
Adds fluid.fluid-calc function to separate property-specific responsive logic from the nuts and bolts math.

Screenshots

Paul-Hebert

This is looking great @tylersticka !

I left some nits and questions inline. Once you've had a chance to review those and address any requiring feedback I'd be happy to approve. Thanks!

Paul-Hebert · 2020-05-04T16:26:59Z

src/components/heading/heading.scss

+$permalink-radius: sizes.$radius-full;
+$permalink-size: sizes.$control-height;
+
+$min-width-permalink-shift: layout.$max-width-prose + $permalink-size * 4;


These var names don't quite explain to me what they are and how they're used. An additional comment might be nice.

(After reading through the rest of the code, or viewing a demo it's clear, but up top where these are placed it's not)

Added more context to this

src/components/heading/heading.scss

Paul-Hebert · 2020-05-04T16:34:06Z

src/components/heading/heading.scss

+ * to the inner heading to avoid unintended font sizes.
+ */
+
+.c-heading__element {


I wonder if there's something more specific than __element we could use here? __text? __content?

I started with __text but changed it because, to me anyway, text suggests that HTML is not allowed. I changed it to __element because this is meant to be specifically the heading element, and c-heading__heading-element felt silly.

I'm open to __content, though. I also toyed with __inner.

I'd be fine with content or inner. I think those communicate more intent than element

Renamed to c-heading__content

src/components/heading/heading.twig

Paul-Hebert · 2020-05-04T16:43:19Z

src/mixins/_fluid.scss

@@ -1,29 +1,47 @@
 @use "unit";

 /**
- * Dynamically adjust font size between a minimum and maximum, starting at a
- * minimum breakpoint width and capping at a maximum.
+ * Mixins and functions that allow for dynamically adjust a CSS property from a


I think there's a typo in this sentence. for doesn't seem like it fits

Rewrote sentence

Paul-Hebert · 2020-05-04T16:50:03Z

src/mixins/_fluid.scss

  $delta-width: unit.strip($max-width - $min-width);
+  // We use `rem` so the fluidity won't be influenced by parent elements


Just wanted to clarify: The min and max breakpoints should always use rems but the fluid value being calculated could use other units. Is that correct?

Not exactly.

Media queries will always calculate em based on the root element's font-size, which sounds a lot like the behavior of rem. But Safari has issues with rem media queries, which is why it's better to prefer em.

But if we use that same value in the font-size calculation, the em will calculate based on the current (not root) font-size. So we have to convert values applied to the element itself to rem, which will more closely match the behavior of the media query.

Okay, gotcha thanks. It might be helpful to add more of that context in a comment as that wasn't all clear to me.

Removed terse comment

Added more detail to main comment

Paul-Hebert · 2020-05-04T16:50:59Z

src/mixins/_fluid.scss

-@mixin font-size($min-width, $max-width, $min-font-size, $max-font-size) {
-  $delta-font-size: unit.strip($max-font-size - $min-font-size);
+/**
+ * Although the the `calc` statement is only part of the equation, breaking it


You have two thes in this sentence.

What, you don't like The The?

Removed the extra "the"

Paul-Hebert · 2020-05-04T16:58:46Z

src/components/heading/heading.twig

+    <a class="c-heading__permalink" href="#{{permalink_id}}">
+      {% include '../../assets/icons/link.svg.twig' with {
+        class: 'c-icon',
+        role: 'presentation'


I wonder if we should use aria-hidden over role="presentation"?

https://timwright.org/blog/2016/11/19/difference-rolepresentation-aria-hiddentrue/

Replaced role="presentation" with aria-hidden="true"

derekshirk · 2020-05-04T17:06:27Z

I'm not sure how I feel about the focus style of the permalink overlapping a (small portion) of the heading text. 🤔

Is this common?

Did you consider animating the heading text with transform: translateX() a little bit to the right when the permalink is focused, eliminating the overlap?

tylersticka · 2020-05-04T17:33:18Z

I'm not sure how I feel about the focus style of the permalink overlapping a (small portion) of the heading text. 🤔

I thought it was kind of cute, but maybe that's just me?

derekshirk

I'm not sure how I feel about the focus style of the permalink overlapping a (small portion) of the heading text. 🤔

I thought it was kind of cute, but maybe that's just me?

I'm not opposed to it and would be happy to approve it as-is from a design perspective. I'm excited to use this new heading component.

tylersticka · 2020-05-04T22:14:03Z

@Paul-Hebert @derekshirk Ready for re-review!

Paul-Hebert

Thanks for making those changes. Looks good @tylersticka ! 🚢

tylersticka added 5 commits April 30, 2020 14:46

Fix fluid type function

3383896

More dynamic heading size logic

f8ba633

Add WIP c-heading

a13e903

Add and clearly document heading component

9af2044

Remove unused knobs

7303cbb

tylersticka requested review from a team May 1, 2020 22:00

tylersticka added ✨ enhancement New feature or request 🎨 design Requires visual, UX or UI design decisions 🦮 a11y Accessibility improvements labels May 1, 2020

Paul-Hebert reviewed May 4, 2020

View reviewed changes

Merge branch 'v-next' into feature/heading-improvements

ddedb53

derekshirk self-requested a review May 4, 2020 17:59

derekshirk previously approved these changes May 4, 2020

View reviewed changes

tylersticka added 6 commits May 4, 2020 14:58

Add explanatory comments to c-heading Sass vars

1996048

c-heading__element → c-heading__content

b5b9774

Fix typos

b235dd3

More detailed em/rem comment

7dde2a0

role="presentation" → aria-hidden="true"

e0bc32c

Merge branch 'v-next' into feature/heading-improvements

0f1cbb4

tylersticka dismissed derekshirk’s stale review via 0f1cbb4 May 4, 2020 22:07

tylersticka requested review from Paul-Hebert and derekshirk May 4, 2020 22:14

Paul-Hebert approved these changes May 5, 2020

View reviewed changes

tylersticka merged commit db2cfd2 into v-next May 5, 2020

tylersticka deleted the feature/heading-improvements branch May 5, 2020 16:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Heading component and fluid size improvements #679

Heading component and fluid size improvements #679

tylersticka commented May 1, 2020

Paul-Hebert left a comment

Paul-Hebert May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

tylersticka May 4, 2020

Paul-Hebert May 4, 2020

tylersticka May 4, 2020

derekshirk commented May 4, 2020

tylersticka commented May 4, 2020

derekshirk left a comment

tylersticka commented May 4, 2020

Paul-Hebert left a comment

		$delta-width: unit.strip($max-width - $min-width);
		// We use `rem` so the fluidity won't be influenced by parent elements

Heading component and fluid size improvements #679

Heading component and fluid size improvements #679

Conversation

tylersticka commented May 1, 2020

Overview

Screenshots

Paul-Hebert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekshirk commented May 4, 2020

tylersticka commented May 4, 2020

derekshirk left a comment

Choose a reason for hiding this comment

tylersticka commented May 4, 2020

Paul-Hebert left a comment

Choose a reason for hiding this comment