cfpb / design-system-react

A React/Storybook implementation of CFPB's Design System
https://cfpb.github.io/design-system-react/
MIT License
6 stars 4 forks source link

fix(hero): let headingLevel dictate the heading tag without enclosing <p> tag #329

Closed billhimmelsbach closed 5 months ago

billhimmelsbach commented 5 months ago

A little bit of a nuanced issue here that affects accessibility. Right now, if I wanted the heading of a hero to be a <h1> instead of a <p> tag, I could pass in the full node instead of the text:

Instead of this: heading={My super cool heading!} Do this: heading={<h1>My super cool heading!</h1>}

The problem with that is that then the heading still renders inside of a <p> tag, which isn't great, but also the stylings of the heading wouldn't get applied to the heading but instead get applied to the enclosing <p> tag.

So this PR instead allows the headingLevel to create a heading with the heading content directly, instead of being wrapped in a <p> tag. This lets you set the heading to be an <h1> or other heading level a little easier, which we'll want for accessibility and also matches the markdown of the DS a little better by removing the enclosing <p> tag. I've also done something similar for the subheading but also it defaults to being a <p> tag.

I used createElement to do this because it's the easiest to implement with Typescript. There are ways of doing this with maybe cloning the element that gets passed to the heading node and then passing it the heading classes... but I think is probably a way that will guide people to creating accessible heroes and it follows the DS in making heroes usually just have the primary headings as h1s.

Closes: #328

EDIT 4/1: I also am exploring a different way of handling this, by just using the <Heading> component directly. Here's the commit on an experimental branch, so please take a peek at that too @meissadia and let me know which you prefer. 👍

Changes

How to test this PR

  1. Do Heros now render the heading and subheading with the element dictated by the headingLevel?

Screenshots

Before when trying to make a hero heading an h1 by passing in <h1>Test</h1> or <Heading> into heading (style is incorrect and wrapped in <p>

Screenshot 2024-03-31 at 3 56 30 PM

Before when trying to make a hero heading by just passing in the heading as "Test" (renders with p tag)

Screenshot 2024-03-31 at 3 58 23 PM

After PR with passing "Test" as heading

Screenshot 2024-03-31 at 3 55 36 PM
netlify[bot] commented 5 months ago

Deploy Preview for cfpb-design-system-react ready!

Name Link
Latest commit a70ca3e6cafd0c939c6390fad3ea68dd9b459674
Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/660bb02312ab770008268ee1
Deploy Preview https://deploy-preview-329--cfpb-design-system-react.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

billhimmelsbach commented 5 months ago

I also am exploring a different way of handling this, by just using the <Heading> component directly. Here's the commit on an experimental branch, so please take a peek at that too @meissadia and let me know which you prefer. 👍

meissadia commented 5 months ago

I also am exploring a different way of handling this, by just using the <Heading> component directly. Here's the commit on an experimental branch, so please take a peek at that too @meissadia and let me know which you prefer. 👍

@billhimmelsbach I like what you've got going in the exploration commit. Makes it more maintainable since any <Heading> changes will get propagated to the Hero component headings as well.

billhimmelsbach commented 5 months ago

I also am exploring a different way of handling this, by just using the <Heading> component directly. Here's the commit on an experimental branch, so please take a peek at that too @meissadia and let me know which you prefer. 👍

@billhimmelsbach I like what you've got going in the exploration commit. Makes it more maintainable since any <Heading> changes will get propagated to the Hero component headings as well.

Sounds good to me!