-
Notifications
You must be signed in to change notification settings - Fork 10
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(css): export mistica tokens as public css custom properties #1161
Conversation
Size stats
|
Accessibility report ℹ️ You can run this locally by executing |
Screenshot tests report ✔️ All passing |
Deploy preview for mistica-web ready! ✅ Preview Built with commit 86af5ae. |
button { | ||
color: var(--mistica-color-textButtonPrimary); | ||
background: var(--mistica-color-buttonPrimaryBackground); | ||
border-radius: var(--mistica-border-radius-button); | ||
font-size: var(--mistica-font-size-2); | ||
line-height: var(--mistica-line-height-2); | ||
font-weight: var(--mistica-font-weight-button); | ||
border: 1.5px solid transparent; | ||
min-width: 104px; | ||
padding: 12px 16px; | ||
cursor: pointer; | ||
outline: none; | ||
} | ||
|
||
button.small { | ||
padding: 6px 12px; | ||
min-width: 80px; | ||
} | ||
|
||
button:hover { | ||
background: var(--mistica-color-buttonPrimaryBackgroundHover); | ||
} | ||
|
||
button:active { | ||
background: var(--mistica-color-buttonPrimaryBackgroundSelected); | ||
} |
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 could be relatively simple to include some basic classes for some components, like buttons or tags inside mistica-common.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.
Nice, also export Boxed style could be nice
I have doubts with this mistica-common file
- Need maintenance?
- If we only include "some" could be open the door to force us to complete more this file
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.
something like this?
.mistica-boxed {
border: 1px solid var(--mistica-color-border);
border-radius: var(--mistica-border-radius-container);
background: var(--mistica-color-backgroundContainer);
}
Implementing inverse logic could be too much
- Yes
- Good point. I think, in case we include some basic components, we need to be clear about the scope and limitations of this. We can provide classes for basic stuff (components made by a single html element), but not for complex components like cards, because they are built by different pieces (title, subtitle, actions, etc) and we'd need classes for all the internal elements and the author would also need to replicate the correct html structure, so this doesn't scale. The most we can/should do in this cases is provide some examples, like the card I built here
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 like it!
You can also include .mistica-boxed-inverse
but this is specifically what I was saying in point 2. xDD you can feed this document to infinity
examples/css/index.html
Outdated
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.
This is an example of use. See this file and sytle.css
.join('\n'); | ||
|
||
const textPresets = { | ||
1: { |
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've extracted these values from text.tsx
, but I think these tokens should be in https://github.com/Telefonica/mistica-design/blob/production/tokens/movistar.json#L1384
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.
Agree we can add this definition in the tokens.json files
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, reviewing the values there's something weird, text4 and text5 have the same size and line height, and it seems that it starts in text 3, that should be size: 16px mobile and 18 desktop
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.
/* Responsive layout */ | ||
.mistica-responsive-layout { | ||
padding-left: env(safe-area-inset-left); | ||
padding-right: env(safe-area-inset-right); | ||
margin: 0 var(--mistica-responsive-layout-margin); | ||
--mistica-responsive-layout-margin: 16px; | ||
} | ||
|
||
@media (min-width: 768px) { | ||
.mistica-responsive-layout { | ||
--mistica-responsive-layout-margin: 24px; | ||
} | ||
} | ||
|
||
@media (min-width: 1024px) { | ||
.mistica-responsive-layout { | ||
--mistica-responsive-layout-margin: 40px; | ||
} | ||
} | ||
|
||
@media (min-width: 1368px) { | ||
.mistica-responsive-layout { | ||
--mistica-responsive-layout-margin: calc((100vw - 1224px) / 2); | ||
} | ||
} |
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.
Does it make sense to include some basic utility classes like this here?
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.
At least a basic one like responsive-layout, yes. But I wouldn't add many of them
css/mistica-common.css
Outdated
} | ||
|
||
/* border-radius utility classes */ | ||
.mistica-border-radius-avatar { |
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'm not sure if border-radius utility classes are worth, maybe having the css vars is enough. Wdyt?
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.
What is the use case? It seems like the CSS var could be enough
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.
agree. If we need the class in the future, we can add it, but for now I think we can live with the vars
src/skins/tu.tsx
Outdated
badge: palette.red70, | ||
badge: palette.blue30, |
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.
is this change intended? I think the is't enough color contrast to read the number inside the badge 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.
It is, badges are blue in TU brand by design. They have a 6+:1 contrast, so it souldn't be a problem.
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.
(anyway, TU dark skin is not ready to use)
font-size: var(--mistica-font-size-title2); | ||
line-height: var(--mistica-line-height-title2); | ||
font-weight: var(--mistica-font-weight-title2); | ||
} |
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'm wondering if we should add title3 to tokens.json so users can have all title sizes available. WDYT @yceballost
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.
Fine by me! and we will need to include also title4
soon, right?
color: var(--mistica-color-textButtonPrimary); | ||
background: var(--mistica-color-buttonPrimaryBackground); |
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.
you could define button as a colorless component and over that, build button-primary, button-secondary
|
||
Naming convention: | ||
|
||
- `--mistica-border-radius-{radiusName}` |
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 think we decided not to create these tokens, right?
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... no, we decided to remove the border-radius utility classes, but we keep the custom properties
# [15.15.0](v15.14.0...v15.15.0) (2024-07-16) ### Bug Fixes * **overscroll color:** background color not changing when switching dark/light mode ([#1172](#1172)) ([9b74fe6](9b74fe6)) ### Features * **Cards:** add role prop for touchable cards ([#1169](#1169)) ([a2a7cbf](a2a7cbf)) * **css:** export mistica tokens as public css custom properties ([#1161](#1161)) ([c5f74bc](c5f74bc)) * **Header:** support headline (Tag) ([#1174](#1174)) ([07d8fae](07d8fae)) * **Text:** support setting heading level to span and header pretitleAs ([#1170](#1170)) ([8abf43e](8abf43e))
🎉 This PR is included in version 15.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
WEB-1703