github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
Closed pretzelhammer closed 2 years ago
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
github-actions[bot]
commented
2 years ago | File | Before | After |
|---|---|---|
components/Button/script.js |
6.2 kB |
-0.96%↓6.2 kB |
components/Heading/index.js |
46 B |
— |
components/Heading/script.js |
4.7 kB |
— |
components/Heading/styles.css |
5.1 kB |
— |
components/Text/script.js |
4.5 kB |
3%↑4.6 kB |
components/Text/styles.css |
5.1 kB |
-6.96%↓4.7 kB |
components/Theme/script.js |
5.5 kB |
19%↑6.5 kB |
utils/css-validator.js |
— | 933 B |
| Total (Includes all files) | 1.3 MB |
-2.65%↓1.2 MB |
| Tarball size | 235.6 kB |
-2.36%↓230.1 kB |
🤖 This report was automatically generated by pkg-size-action
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
landondurnan
commented
2 years ago Before merging this breaking change we should understand that there's existing gaps with our theme output when it comes to our default font-family and weights. Every other component we have that will leverage the font family, weights, and sizes will also need to be updated. Buttons, forms, etc all should reflect the default output as well and it doesn't seem likely that we will want to (or in some cases even can) embed a text component into every other component that needs to have the same styling.
While this PR resolves the larger issue we have with regular heading and text elements -- we need to think more holistically here. If a breaking change is inevitable we need to at the very least do it as one major release since each one of these will increasingly create more disruptions as we work on other non-breaking changes.
You can see a simple example of this issue in our Theme demo: https://square.github.io/maker/styleguide/latest/#/Theme
landondurnan
commented
2 years ago Discussed a bit more synchronously and the primary concern I have with this PR is the breaking change (and our frequency of them recently). I think a fair compromise at this point is to attempt to amend this code so it's non-breaking. Leaving the Heading component as-is and providing a bit more time for us to leverage this solution in practice.
The variant approach is useful and will be a helpful pattern in other components.
One other questions -- why is it necessary to set inherit as the default on so many properties? Shouldn't the default browser behavior already do this?
bashunaimiroy
commented
2 years ago I took another look and overall this would be super helpful, especially the behaviour where passing inherit for a style-related prop (e.g. color="inherit") will prevent it from getting added to inline styles. This addresses my concern here (slack link), although mostly I see why we'd prefer the "friction" factor of using the color prop explicitly and I'll be favouring that.
So that specific improvement in this PR would make my Text to MText Conversion work easier (internal github link), and the text variant improvements in general would make everything better and clarify how we add text to the Website. I don't have an opinion on the breaking change of removing MHeading, but it won't prevent my work. I endorse this. I added a minor code question, otherwise I'm ready to approve this PR.
landondurnan
commented
2 years ago Looks like there is an issue with the markdown table generation:
The component also looks to have default element and sizes, but those don't show up in the markdown table as defaults.
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
github-actions[bot]
commented
2 years ago Styleguide deployed to https://square.github.io/maker/styleguide/text-variants/#/
pretzelhammer
commented
2 years ago @landondurnan
Since we don't have a way right now to release non-breaking bug fixes to multiple major releases with our semantic release jobs.
I think we might have a way to do this. Semantic release has documentation on how to do this. I'll experiment backporting a v9 bugfix to v8.
- Release this as a beta until we get closer to a point we know it won't be disruptive with other ongoing work.
Okay sure, I changed the base of this PR from master to beta.
landondurnan
commented
2 years ago I have concerns that we keep changing the way we're passing theme data. Variants will be useful, but we should support a global definition with the fonts.
Our current data looks something like this:
"heading": {
"fontFamily": "Karla",
"fontWeight": "600"
},
"text": {
"fontFamily": "Domine",
"fontWeight": "400"
},
"fonts": {
"baseSize": "18",
"sizeScale": "1.327"
}But in this PR its changing again after we just defined this spec in the v9 release that just came out a little over a week ago.
I think this approach by defining the component + variant as the way to style specific components is powerful for advanced used cases, but limiting in that it only styles those components.
We have numerous components that would benefit from a more generic global definition. Most of our components should inherit what's currently defined as the text.fontFamily + text.fontWeight. Then we also want the fonts.baseSize on many components or the ability to leverage one of the generated step sizes that are being generated in the Text component.
So my concern that I originally brought up about how we're changing the data structure for theming is still relevant because the way things are going is going to require every component to have props to change simple defaults for the font family, weight, and size.
What I have been recommending prior to our v9 release and again before we're looking at introducing yet another breaking change to the theme data structure is a definition based on this approach: https://theme-ui.com/theme-spec/
By leveraging this spec we can have simple global definitions (which would be sufficient for our current use cases) and support the variants for advanced use cases later.
fonts: {
body: 'system-ui, sans-serif',
title: 'system-ui, sans-serif',
headline: 'system-ui, sans-serif',
label: 'system-ui, sans-serif'
baseSize: 16,
sizeScale: 1.15
},
fontWeights: {
body: 400,
title: 700,
headline: 400,
label: 400,
bold: 700
},Using this data could be additive and non-breaking. We can still do the variant approach as a way to do additional customizations in unique scenarios, but it would alleviate the need to define every prop on every components to pass a few CSS styles.
github-actions[bot]
commented
2 years ago :tada: This PR is included in version 10.0.0-beta.1 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
Describe the problem this PR addresses
Describe the changes in this PR
h1-h6tagsheadline,title,body, andlabelvariantpropelement,fontStyle,textTransform, andtextAlignthemeablecssValidatorutil which is a factory function which produces validators for props which are just CSS styles, and updates thefontFamily,fontWeight,fontSize,lineHeight,color,fontStyle,textTransform, andtextAlignprops to usecssValidator. changesfontWeightandlineHeightfrom Numbers to Strings to accommodate wider range of now supported CSS values.inheritstylesBulk of the work in done in these key files:
Theming MText (& MHeading) before this PR:
Theming MText after this PR:
Making Headlines, Titles, Body text, and Label text before this PR:
Making Headlines, Titles, Body text, and Label text after this PR:
Migration Guide
<m-heading>s in your code to<m-text variant="title">pelement, so you may have to add margin back to those components in your app