Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Component] Heading #175

Merged
merged 12 commits into from
Sep 29, 2023
Merged

[Component] Heading #175

merged 12 commits into from
Sep 29, 2023

Conversation

meissadia
Copy link
Contributor

@meissadia meissadia commented Sep 19, 2023

Closes #161

Changes

  • feat: Ability to create h1 - h5 elements
  • feat: Ability to create "Display heading"
  • feat: Ability to create "Eyebrow heading"
  • feat: Ability to create "Slug heading"
  • feat: Icon w/ Text table on the Icons page

How to test this PR

  1. yarn vitest Heading

Screenshots

Updated: Sept 29

Sidebar

Screenshot 2023-09-29 at 9 24 38 AM

Overview

screencapture-localhost-6006-2023-09-29-09_24_40

Icons with Text (live preview) - (On the Iconography page)

Screenshot 2023-09-26 at 2 09 59 PM

Notes

@netlify
Copy link

netlify bot commented Sep 19, 2023

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

Name Link
🔨 Latest commit 9f75357
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/6516f5568a1a2e0009e45830
😎 Deploy Preview https://deploy-preview-175--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.

shindigira
shindigira previously approved these changes Sep 19, 2023
Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, dynamic components!

@natalia-fitzgerald
Copy link

@meissadia
Can we change the name of the "Superheading" to "Display"? I see in the DS this is labeled with a class of "superheading" but this is not the way we reference this heading ever. On the website and in print it is always referred to as "Display heading" or "Display." If we wanted to change this on the Design System side as well how disruptive do you think that would be?

@meissadia
Copy link
Contributor Author

meissadia commented Sep 19, 2023

@natalia-fitzgerald

Can we change the name of the "Superheading" to "Display"?

Done ✅

Screenshot 2023-09-19 at 4 17 16 PM

If we wanted to change this on the Design System side as well how disruptive do you think that would be?

Assuming this would be changing the classname from "superheading" to "displayheading" or similar? It seems like a small change in the DS as there are only two places that reference "superheading".

As for the downstream implications, replacing the "superheading" class would break styles for any apps that rely on it. Since it would be a breaking change this would require a new major version release of the DS library.

Alternatively we could start a transition phase by duplicating the "superheading" styles as "displayheading", updating the DS docs page to prefer to the new classname, and maybe add a warning that the "superheading" class will be deprecated in future versions of the DS.

@natalia-fitzgerald
Copy link

@meissadia - Since “Heading with icon” is included on the Design System page for “Headings” we should consider including it here. Or do we currently include it somewhere else in the Storybook library? In the CFPB Design System we include “Heading with icon” on both the “Headings” page and on the “Iconography” page.

@meissadia
Copy link
Contributor Author

Since “Heading with icon” is included on the Design System page for “Headings” we should consider including it here. Or do we currently include it somewhere else in the Storybook library? In the CFPB Design System we include “Heading with icon” on both the “Headings” page and on the “Iconography” page.

Can do. I think it would stay in the "Headings" section since it's a variation on how the heading element is composed. And I don't see it on the Iconography.

I do see the Icons with text section, which could be implemented as it's own component and could help solve the Consistent Icon/Text spacing issue.

Question: Should our implementation be the whole meta header or just the "heading" (that is implemented in the DS as an anchor element) that includes an icon?

@meissadia meissadia force-pushed the 167-headings branch 2 times, most recently from 17ecfde to dfa7c17 Compare September 21, 2023 19:52
@meissadia
Copy link
Contributor Author

  • Still discussing if we should include Heading w/ Icon or if that should be saved for a future MetaHeader component
  • Only h1-h6 were requested in original issue, should we remove Slug & Eyebrow?

@meissadia meissadia force-pushed the 167-headings branch 2 times, most recently from 3ed90b8 to 0838d62 Compare September 21, 2023 21:02
@shindigira shindigira self-requested a review September 21, 2023 21:28
@shindigira shindigira dismissed their stale review September 21, 2023 21:29

More pending issues to review

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meissadia
We should include the Eyebrow and the Slug since they are standard components and could come up as we build out more pages for SBL. The Slug is mainly used for sidebar content headings. We could handle Icons + Text as a separate PR if you'd like. I would like to discuss that one in more detail but I don't want to hold up the rest of this PR.

@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Sep 22, 2023

@meissadia
I read through the “Heading with icon” content on the CFPB Design System and I think that it refers only to that specific implementation that typically appears within the results of a filterable list. I think that the CFPB Design System name for this component should be changed to something more specific, because “Heading with text” is more aligned with what we currently refer to as “Icons with text.” Next week I’ll post a CFPB DS issue to change the name of this component within the Design System.

What is the “.a-heading__icon”pattern that is referenced in the implementation details? Is this an icon next to an Avenir Next 18px Medium heading? Since this doesn’t use the H4 element does it not get the mobile styling for an H4?

In terms of whether we should add it to the React component library now I’d say that we should but hold off until after we have the naming discussion in the DS repo. Or we can wait for a known use case in the SBL app. Whichever of these things happens first.

@meissadia
Copy link
Contributor Author

Per conversation with Natalia this a.m.:

  • We're holding off on implementing the Heading with icon variant
  • I've verified that Headings sizes adjust (shift to a smaller font-size) on at mobile device widths
  • Screenshots in PR description are up-to-date

@natalia-fitzgerald
Copy link

@meissadia

  • Change "Heading level 1" to "Heading 1" and make this change globally to all heading levels.
  • Change "Display heading" to "Display"

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meissadia - I left some comments in a separate post. Just minor content edits.

@meissadia
Copy link
Contributor Author

@meissadia

  • Change "Heading level 1" to "Heading 1" and make this change globally to all heading levels.
  • Change "Display heading" to "Display"

@natalia-fitzgerald

Updated:

Sidebar

Screenshot 2023-09-28 at 1 49 56 PM

Stories

screencapture-localhost-6006-2023-09-28-13_52_04

@natalia-fitzgerald
Copy link

@meissadia
The content changes look good!

@natalia-fitzgerald
Copy link

@meissadia
There is one more change to the headings page. Please remove the H6 from the type hierarchy. This is being deprecated from the CFPB DS and should not be used here.

@natalia-fitzgerald
Copy link

@meissadia
Moving this from our chat convo: For Headings, change the example to show Heading 1 instead of Display. It is fine to move Display out of the first position to make this possible.

@meissadia meissadia merged commit 976a165 into main Sep 29, 2023
@meissadia meissadia deleted the 167-headings branch September 29, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Component] Headings (1-6)
3 participants