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

Style section highlights headers as h2/h3 #2203

Merged
merged 2 commits into from
May 21, 2024

Conversation

RoyEJohnson
Copy link
Contributor

@RoyEJohnson RoyEJohnson commented Mar 25, 2024

DISCO-43
I made the Popup Header an h1 so that we have the correct sequence of headers.
It made no sense to me that it was an h3 previously. It is a major, separate section. If we think it should be h3, then the section headers need to become h4 and h5.

@RoyEJohnson RoyEJohnson requested a review from a team as a code owner March 25, 2024 21:41
@RoyEJohnson RoyEJohnson requested a review from jivey March 25, 2024 21:41
@TomWoodward TomWoodward temporarily deployed to rex-web-study-guide-sec-8izfli March 25, 2024 21:41 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-study-guide-sec-8izfli March 26, 2024 20:31 Inactive
Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

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

This is a good change. I'm wondering about the awkwardness of the h3Style on the h1. Maybe we should update the heading style names to be more abstract like large/medium/small? Or add aliases?

@RoyEJohnson
Copy link
Contributor Author

RoyEJohnson commented Mar 29, 2024

I'm conflicted on whether it's good to have the association between h1 and a particular size. We do tend to think that way. It makes some sense to say "this will be styled like we normally style an h3 even though it's structurally an h1".
In the Typography/headings file there are five sizes spelled out: h1-h4, plus h4-mobile (also h3-mobile, which is the same as h4-mobile). H1 and H2 are only defined on styled h1 and h2.
They could be largest, large, medium, small, and smallest. Would that help with decision making about which to use? Would we mentally convert to h-something anyway?

Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

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

Yeah we could totally make a case for either way... I think I mentally tend to separate the style from the markup (though hopefully they match in relative scale) because I'm used making small tweaks in different areas anyway. But with REX I don't have a good sense of how these are used, so I think this is a good way to handle it. And in the future we might replace some of these with typography helpers from ui-components.

@Malar-Natarajan Malar-Natarajan temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 16, 2024 18:34 Inactive
@Malar-Natarajan Malar-Natarajan temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 16, 2024 20:04 Inactive
@Malar-Natarajan Malar-Natarajan temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 17, 2024 17:44 Inactive
@Malar-Natarajan Malar-Natarajan temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 17, 2024 18:19 Inactive
@TomWoodward TomWoodward temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 17, 2024 19:17 Inactive
@RoyEJohnson RoyEJohnson force-pushed the study-guide-section-headings branch from fe06a77 to 96875f4 Compare May 17, 2024 19:18
@TomWoodward TomWoodward temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 17, 2024 19:18 Inactive
@RoyEJohnson RoyEJohnson force-pushed the study-guide-section-headings branch from 96875f4 to b8b0aa2 Compare May 17, 2024 19:26
@TomWoodward TomWoodward temporarily deployed to rex-web-study-guide-sec-9nxiv7 May 17, 2024 19:26 Inactive
@RoyEJohnson
Copy link
Contributor Author

@jivey I added a commit here to pull the put-away X out of the popup header. Thought you might want to look it over. I had to change Header to be a div and then put the h1 tags explicitly around the heading text.

@RoyEJohnson RoyEJohnson requested a review from jivey May 17, 2024 19:39
Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

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

That's a nice idea for handling the h1 in Header 👍

@Malar-Natarajan Malar-Natarajan merged commit b173394 into main May 21, 2024
8 of 9 checks passed
@Malar-Natarajan Malar-Natarajan deleted the study-guide-section-headings branch May 21, 2024 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants