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

refactor: make main heading font size changeable via CSS var #5377

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Aug 18, 2021

Motivation

Instead of changing font size of main header via font-size property, it should only be possible to change related --ifm-h1-font-size CSS var. This is better approach also allows us to get rid of one extra class.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview (actually, there is no visible changes)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Aug 18, 2021
@lex111 lex111 requested a review from slorber as a code owner August 18, 2021 14:02
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 18, 2021
@@ -24,7 +24,7 @@ export const MainHeading: HeadingComponent = function MainHeading({...props}) {
<h1
{...props}
id={undefined} // h1 headings do not need an id because they don't appear in the TOC
className={styles.h1Heading}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we removed h1Heading class, should this change be marked as BC?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to prefer reporting breaking changes even if tiny,

But I'd say CSS module classes do not need to be reported as breaking changes as it would only add noise.
It's somehow implicit it is internal code (similarly to the HTML tree) and the user probably understand this, as it's not easy to create a css module selector in the first place.

@netlify
Copy link

netlify bot commented Aug 18, 2021

✔️ [V2]

🔨 Explore the source changes: 96f42a2

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/611d1309553183000778458c

😎 Browse the preview: https://deploy-preview-5377--docusaurus-2.netlify.app

@github-actions
Copy link

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 70
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5377--docusaurus-2.netlify.app/

@github-actions
Copy link

Size Change: -349 B (0%)

Total Size: 804 kB

Filename Size Change
website/build/assets/css/styles.********.css 93.6 kB -250 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 36.2 kB 0 B
website/build/assets/js/main.********.js 405 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 65.3 kB 0 B
website/build/blog/index.html 35.4 kB 0 B
website/build/docs/index.html 43.1 kB -26 B (0%)
website/build/docs/installation/index.html 50.7 kB -27 B (0%)
website/build/index.html 29.8 kB 0 B
website/build/tests/docs/index.html 23.6 kB -23 B (0%)
website/build/tests/docs/standalone/index.html 21.5 kB -23 B (0%)

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Aug 19, 2021

thanks 👍

@slorber slorber merged commit 974644d into main Aug 19, 2021
@slorber slorber deleted the lex111/infima-31-h1 branch August 19, 2021 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants