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
v7 - Remove su- prefix; update sans and serif fonts; add variant group-hocus-within: #918
Conversation
…pro first; update google font import; remove su- in docs
@@ -1,8 +1,7 @@ | |||
@charset "UTF-8"; | |||
|
|||
/** Source Sans Pro, Source Serif Pro, Roboto Slab, Roboto Mono (regular only) */ | |||
@import url('https://fonts.googleapis.com/css2?family=Roboto+Mono&family=Roboto+Slab:wght@300;400;700&family=Source+Sans+Pro:ital,wght@0,300;0,400;0,600;0,700;1,300;1,400;1,600;1,700&family=Source+Serif+Pro:ital,wght@0,300;0,400;0,600;0,700;1,300;1,400;1,600;1,700&display=swap'); | |||
|
|||
@import url('https://fonts.googleapis.com/css2?family=Roboto+Mono&family=Roboto+Slab:wght@300;400;700&family=Source+Sans+3:ital,wght@0,400;0,600;0,700;1,400;1,600;1,700&family=Source+Serif+4:ital,wght@0,400;0,600;0,700;1,400;1,600;1,700&display=swap'); |
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.
These font imports are only used for the preview site here; I replaced our sans and serif fonts, plus got rid of the 300 font weight since we never use them in the preview and won't (we don't even use that for our projects even).
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.
https://github.com/SU-SWS/saa_alumni/blob/dev/src/styles/global.css#L1
We totally can and do use the fonts.css file in projects
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.
@sherakama You're right we need to update the font import in the index.css file. I only updated it here in this font.css file 😬 Good catch.
@@ -5,13 +5,15 @@ module.exports = function () { | |||
return { | |||
mono: ['"Roboto Mono"', 'Menlo', '"Courier New"', 'monospace'], | |||
sans: [ | |||
'"Source Sans 3"', |
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.
Adding the SS3, but leaving the SS Pro as 2nd place for now
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.
Why?
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.
Also,
Don't we need to update the import as well?
https://github.com/SU-SWS/decanter/blob/v7/src/css/index.css#L4
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.
Just in case someone installs this latest version of Decanter but missing the note about the font change in the projects. They have to also update the font import in their own projects, or else it will render the other fall back font like Arial 😬
It might not be obvious when they preview locally because the serif and sans fonts are not super different from the default fonts (times new romans or arial etc)
Open to suggestions though 😄
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 don't think it hurts to have the fallback and that makes sense with what you are saying on the expectation of the project importing the fonts.
'"Source Sans Pro"', | ||
'"Helvetica Neue"', | ||
'Helvetica', | ||
'Arial', | ||
'sans-serif', | ||
], | ||
serif: [ | ||
'"Source Serif 4"', |
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.
Same for the serif font - add the new, but leave the old one as 2nd place
// Our own prefix. | ||
prefix: 'su-', | ||
|
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.
addVariant('group-hocus-within', [ | ||
':merge(.group):focus-within &', | ||
':merge(.group):hover &', | ||
]); |
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.
Tested this out in Campaign and added for convenience
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.
GTG. I've added a minor
tag to this PR as it does require the downstream consumer to make change.
Thanks @sherakama - totally agree about the minor tag 😄 |
READY FOR REVIEW
Summary
su-
prefix due to popular demandgroup-hocus-within
: variant (group-hover:
+group-focus-within:
)su-
prefixSteps to Test
Associated Issues and/or People