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

Remove blankslate heading styling #969

Closed
wants to merge 3 commits into from

Conversation

pouretrebelle
Copy link
Member

@pouretrebelle pouretrebelle commented Jan 7, 2022

Primer::Beta::Blankslate forces .h2 styling on the heading slot even if a non-h2 tag is passed in.

This was flagged by @rewinfrey on Slack, a lot of confusion was caused by the h4 tag having a .h2 class, and since .h2 uses !important declarations it couldn't be overridden.

The migration script to beta defaults to a h2 tag, so there are only a handful of examples that specify a different tag; in these instances the font size would be reduced with this change.

pouretrebelle and others added 3 commits January 7, 2022 16:36
This has caused issues where consumers have used the h4 tag and been
confused that the font-size isn't adjusted. The .h2 utility tag adds
!important rules which we want to avoid because we can't override
them.
@vercel
Copy link

vercel bot commented Jan 7, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/view-components/8eKdMxNtgoW7PMtVkm8Af9g6nLaB
✅ Preview: https://view-components-git-pouretrebelle-fix-blankslate-title-primer.vercel.app

@pouretrebelle
Copy link
Member Author

Question for @ashygee - do we want to maintain the h2 sizing even if a different tag is passed in? I'm guessing that was the original intention but we'll have to add a .blankslate-heading class to Primer CSS to do that instead of using the utility (we'll have to do that as part of https://github.com/github/primer/issues/585 anyway to remove the margin utility).

@pouretrebelle pouretrebelle marked this pull request as ready for review January 7, 2022 17:49
@pouretrebelle pouretrebelle requested review from a team and manuelpuyol January 7, 2022 17:49
@ashygee
Copy link
Contributor

ashygee commented Jan 19, 2022

Question for @ashygee - do we want to maintain the h2 sizing even if a different tag is passed in? I'm guessing that was the original intention but we'll have to add a .blankslate-heading class to Primer CSS to do that instead of using the utility (we'll have to do that as part of github/primer#585 anyway to remove the margin utility).

@pouretrebelle no I don't think we should. At the time of the API audit I had made a recommendation that the default would be an h3 but that the heading size could be changed depending on the hierarchical placement of the blankslate. The original intention of the blankslate, in my mind, was to be a placeholder in larger, more spacious UI. Thus a larger h3 or h2 heading size felt appropriate. But with the discovery of the usage in more condensed UI and the fact that we do not have an alternative solution at this time, I would say that we can and should be flexible with the heading sizing.

@pouretrebelle
Copy link
Member Author

Superseded by #1000

@jonrohan jonrohan deleted the pouretrebelle/fix-blankslate-title branch May 26, 2022 21:48
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.

5 participants