-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(components): add helix product typography #15459
Conversation
export const fontSizeDisplayBold = fontSizeDisplay | ||
export const lineHeightDisplayBold = lineHeightDisplay | ||
export const fontFamilyDisplayBold = fontFamilyDisplay | ||
export const fontWeightDisplayBold = '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.
maybe define font-weight ?
const regular = 400'
const semiBold = '600'
const bold = '700'
export const fontSizeCaptionMedium = fontSizeCaption | ||
export const lineHeightCaptionMedium = lineHeightCaption | ||
export const fontFamilyCaptionMedium = fontFamilyCaption | ||
export const fontWeightCaptionMedium = '500px' |
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.
500?
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.
whoops
export const fontSizeCaptionRegular = fontSizeCaption | ||
export const lineHeightCaptionRegular = lineHeightCaption | ||
export const fontFamilyCaptionRegular = fontFamilyCaption | ||
export const fontWeightCaptionRegular = '400px' |
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.
400?
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.
Whoops
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.
lgtm!
thank you for adding this!
d8d2be7
to
acec934
Compare
*/ | ||
|
||
// Valid for most typography styles | ||
export const fontFamily = 'Public Sans' |
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 we use this in app/src/atoms/GlobalStyle/index.ts
?
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.
use .ts
itead of .tsx
?
// Code | ||
const fontSizeCode = '0.75rem' // 12px | ||
const lineHeightCode = '1.25rem' // 20px | ||
const fontFamilyCode = 'Reddit Mono' |
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 we use Reddit Mono
without installing a package?
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.
Definitely not, or at least without installing a font however we install a font - if you look at storybook, this shows up as whatever the browser fallback is. I really haven't investigated how/whether we can isntall this, so I think that if we don't want to keep it as a placeholder I'd rather delete it 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.
For Public Sans, app
is using @fontsource
and fontsource supports reddit mono
.
// Caption | ||
const fontSizeCaption = '0.8125rem' // 13px | ||
const lineHeightCaption = '1rem' // 16px | ||
const fontFamilyCaption = 'Public Sans' |
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.
const fontFamilyCaption = 'Public Sans' | |
const fontFamilyCaption = 'Public Sans' |
const fontFamilyCaption = 'Public Sans' | |
const fontFamilyCaption = fontFamily |
export const fontSizeCaptionRegular = fontSizeCaption | ||
export const lineHeightCaptionRegular = lineHeightCaption | ||
export const fontFamilyCaptionRegular = fontFamilyCaption | ||
export const fontWeightCaptionRegular = '400' |
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.
export const fontWeightCaptionRegular = '400' | |
export const fontWeightCaptionRegular = fontWeightRegular |
export const fontSizeCaptionMedium = fontSizeCaption | ||
export const lineHeightCaptionMedium = lineHeightCaption | ||
export const fontFamilyCaptionMedium = fontFamilyCaption | ||
export const fontWeightCaptionMedium = '500' |
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.
nit
maybe define as fontWeightMedium and add it to the top fontWeight group?
then
export const fontWeightCaptionMedium = fontWeightMedium
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's not though, fontWeightMedium
is 600
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.
well, fontWeightSemibold
anyway
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.
and we got a dqa update to make it 600, so i did this
c5f4afe
to
32b60f1
Compare
- Adds the new sizes for type in the helix product system - Adds a dummy component under "Design Tokens" in storybook so you can view all the text Closes EXEC-566 Make the storybook display all the stories
LegacyStyledText will use the current styles; a new StyledText implementation will use the helix design system typography.
StyledText is a component that is only really useful if you're goign to do semantic styles. Semantic styles are only really useful if they mean the same thing in all the places they're used. This isn't really true right now, but we can at least have enumerated style tokens that you can select from when creating one. Now, which style token you use is separately selectable across ODD and non-ODD flows. You can still fully or partially override the CSS if you want, but the styles on desktop align with helix and the styles on ODD align with what's going on in ODD.
78614b2
to
051220b
Compare
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.
very nice 🤩
level1Header: { | ||
as: 'h1', | ||
style: css` | ||
@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs} { |
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.
question, do we need these media queries if these are exclusively for the ODD?
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.
Yes because the idea is that you will be passing both a desktopStyle
and oddStyle
to the component, so we want them to work together nicely
This PR implements the new helix product typography system. You can find it here: https://www.figma.com/design/1ot18My22DALIcjdLl5LJh/Helix-Design-System?node-id=3571-46578&t=B4pPbtL9mapki2Jg-4
The Product type system is distinct from at least the Web type system, which will have different styles for readability in a different context. It is currently only defined for places other than the ODD.
This ended up being a big PR because our usage of the
StyledText
atom had to be changed somewhat.The change was necessary because the helix product styles do not and are not intended to align with the semantic styling presented by the
StyledText
component - there is no direct alignment that "when you're making a<p>
, you use this style". Instead, the intent is that higher level components will use the style by the name it has in helix, without an explicit semantic link outside of the designs. That makes the way the old styled text works kind of nonsense.In addition, there are no helix product styles for the ODD, and there is no current map and possibly no intention to have a future map between the semantic styles of the ODD and Desktop. That means that having a
StyledText
component that maps from a single semantic element to a set of appropriate styles just doesn't really make sense.So,
StyledText
is renamed toLegacyStyledText
including at all callsites. The newStyledText
requires you to specify styles separately for ODD and Desktop, though it gives you a handy type system integrated hint towards which styles should be allowed. Well, not quite required, more like you have no way to specify both in one prop.You can view it commit by commit:
StyledText
toLegacyStyledText
StyledText
.Stories here: https://s3-us-west-2.amazonaws.com/opentrons-components/EXEC-566-add-helix-product-typography/index.html?path=/docs/design-tokens-typography--docs
Closes EXEC-566