-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
fix(heading): improve heading style #7628
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Lighthouse Results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- packages/ui-components/styles/markdown.css: Language not supported
- packages/ui-components/styles/theme.css: Language not supported
Co-authored-by: Copilot <[email protected]> Signed-off-by: Augustin Mauroy <[email protected]>
@@ -75,7 +75,8 @@ main { | |||
dark:text-white; | |||
} | |||
|
|||
a, | |||
/* link that isn't inside a heading */ | |||
a:not(h1 > a):not(h2 > a):not(h3 > a):not(h4 > a):not(h5 > a):not(h6 > a), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSS 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah 🤘🏻
@@ -95,7 +94,7 @@ | |||
--font-xs-line-height: 1rem; | |||
--font-sm: 0.87rem; | |||
--font-sm-line-height: 1.25rem; | |||
--font-base: 1rem; | |||
--font-base: 0.9rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, did we not use something else before? What were we using for defining base font size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK maybe coming from codemod ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check what was the original value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodejs.org/packages/ui-components/tailwind.config.ts
Lines 90 to 108 in 5230197
fontSize: { | |
xs: ['0.75rem', '1rem'], | |
sm: ['0.875rem', '1.25rem'], | |
base: ['1rem', '1.5rem'], | |
lg: ['1.125rem', '1.75rem'], | |
xl: ['1.25rem', '1.875rem'], | |
'2xl': ['1.5rem', '2rem'], | |
'3xl': ['1.875rem', '2.25rem'], | |
'4xl': ['2.25rem', '2.5rem'], | |
'5xl': ['3rem', '3rem'], | |
'6xl': ['3.75rem', '3.75rem'], | |
'7xl': ['4.5rem', '4.5rem'], | |
}, | |
fontWeight: { | |
regular: '400', | |
medium: '500', | |
semibold: '600', | |
bold: '700', | |
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked to the Wayback machine and apparently it was always 16px. That's interestingly odd. Let's undo this change then and keep base as 1rem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just feels odd to me that the content font size is larger than, for example, the sidebar or other elements such as tables and so on. What do you all think, @nodejs/nodejs-website?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The font values weren't updated by the codemod, i had to do it manually :(.
I'm trying to figure out what's different compared to what's in production, but I can't spot any difference.
For posterity, can you link the slack discussion? |
Normally it's okay now |
Description
Fix bugs introduced by migration of tailwind v4 or monorepo.
Validation
Related Issues
No related issues just slack discussion
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.