In the original font-size hardcoded styles, there were 11 different font sizes. CSS variables were introduced to standardize writing in a more
readable way, which will have long-term benefits for typography. Line height was also added for <p> tags.
Details
Details
- Reviewers
emack bytesofman johnkuney - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project
npm test
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- master
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 32750 Build 64988: Build Diff cashtab-tests Build 64987: arc lint + arc unit
Event Timeline
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 53 | When line-height is specified without a unit, it represents a multiplier of the font size. Choosing rem is a practice designers prefer because it's based on the root element (html) font size, which helps maintain font proportion relationships. Our size class names are a reproduction of tailwind.css. I've kept the px values because I replaced all the original px units, and I've preserved the comments to help developers understand the relationship between rem and px. These comments can be removed if needed. | |
This comment was removed by alitayin.
Comment Actions
I have streamlined the 11 sizes down to 6. I will continue to improve the font relationships in the future steps. During the process of changing fonts, I merged some similar-sized fonts, for example, 12px and 14px were changed to text-sm, 1.17em and 18px to text-lg, 20px and 22px to text-xl, and 1.5em, 24px, and 26px to text-2xl.
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 53 |
| |
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 53 | Yes. My plan is to first introduce it and see if it's "adopted," then gradually introduce other styles. At the same time, I'll make some adjustments to the current fonts, so it's like a palette. Eventually, only the variables that are used will be kept. However, we can remove it for now if needed. The advantage of using calc is that, for example, with --text-xs--line-height: calc(1 / 0.75);, if you want the line height and font size to have a 4:3 relationship, expressing this with the calculated result would be 1.3333, which isn't very readable. Using calc makes it more readable and clearly shows the proportion relationship. | |
Comment Actions
- If we are switching from px to rem, switch; remove all the px comments, mb keep one to show what the base-level px is
- do not use calc. if some ratios seem unreadable, create a variable for them and use the variable
- remove all unused variables
Comment Actions
this diff was created on the master branch, which makes it difficult to rebase onto the latest master
Please follow this procedure so that this diff is not on master
git checkout -b cashtab-css-vars git branch -f master origin/master git checkout cashtab-css-vars git rebase origin master arc diff
Comment Actions
Good idea, but think theres some over complicating going on with the line heights
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 28 ↗ | (On Diff #53195) | technically this is an assumed 16px as thats the browser default but thats not guaranteed. Lets also set the root font size here to 16px font-size: 16px somewhere in this :root definition |
| 34–40 ↗ | (On Diff #53195) | These line height variable names are too complex I reckon. Also I don't think the line height should be related to the font size. What if you want a small font size with a medium line spacing? Or a large one with tighter spacing. And why is one named leading but not the others? The non unit numbers here will apply the line height as a multiple of whatever font size it is applied to, so no need for font size specific names I think we should just take a page from tailwinds and do something like leading-none --- line-height: 1; but we dont need that many right now. So maybe just --leading-none: 1 |
| cashtab/src/components/Common/Atoms.tsx | ||
| 61 ↗ | (On Diff #53195) | why add a line height definition if there wasnt one before? |
| 65 ↗ | (On Diff #53195) | same here and basically every other place you added the font size variable. Unless it was explicitly set why set it now? |
| cashtab/src/components/Etokens/Token/styled.ts | ||
| 99 ↗ | (On Diff #53195) | This is one of two with an explicit line height already set...could just leave it as a unitless 1.2 as I think the parent element has a 16px font size. Or use a variable with that value |
| cashtab/src/components/Nfts/styled.ts | ||
| 21 ↗ | (On Diff #53195) | this one should be a unitless 1 if you want to keep it more relative units |
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 28 ↗ | (On Diff #53195) | Yes, as mentioned above. Keeping 16px is just for a simple comparison with the previous modification. Using rem is to maintain better flexibility based on the browser's default value. Here, using 1rem is to follow the browser's settings to provide a better typography experience. |
| 34–40 ↗ | (On Diff #53195) | In the Tailwind system, names like text-base are class names that include both font size and line height. From a design specification perspective, fonts should be paired with line heights, which is one of the purposes of this modification. "--leading-tight: 1.25; thoese line-hieght will be used elsewhere, such as in p, h1, h2, while --text-base--line-height is specifically used to text-base. |
| cashtab/src/components/Nfts/styled.ts | ||
|---|---|---|
| 21 ↗ | (On Diff #53195) | I will slightly adjust the spacing when introducing other variables like margin, padding and radius in the next diff. |
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 28 ↗ | (On Diff #53195) | yes rem is a unit that is relative to the font-size of the root element, <html>. The browser default is usually 16px, but if you dont explicitly set it to 16px, its possible it can be something different. So define it as 16px here |
| 34–40 ↗ | (On Diff #53195) | Okay fair enough |
| cashtab/src/components/App/styles.ts | ||
|---|---|---|
| 28 ↗ | (On Diff #53195) | I understand. But introducing rem is precisely to allow fonts to adapt automatically on different devices. If we set the root to 16px, it would defeat this purpose. Adding 16px in the comments is to show that I'm using text-base as a baseline to replace the previous 16px. text-base should be used as body, it's an appropriate rem value. |