-
Notifications
You must be signed in to change notification settings - Fork 21
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(app): add new spacing and colour variables #876
Conversation
Preview branch generated at https://feat-new-variables.d1gko6en628vir.amplifyapp.com |
packages/styles/variables.css
Outdated
@@ -81,6 +88,7 @@ | |||
--text-size-larger: 56px; | |||
--text-size-large: 45px; | |||
--text-size-large-medium: 34px; | |||
--text-size-medium-small: 32px; |
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.
A more appropriate name for this would be --text-side-large-small
or --text-size-medium-large
, since it is a step up from --text-size-medium
(the -small
makes it sound like a step down)
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.
In retrospect, I wish we had gone with a different verbiage because these text sizes are all confusing and don't scale well when adding new ones. That said, I agree with @thuey 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.
Here are my observations:
Frankly, these are bad naming conventions because it's pretty ambiguous what each variable means without having to remember or go look at documentation.
My recommendation in the future is to use naming conventions such as --space-2
, --space-4
, --space-6
, --space-8
, etc. as I've outlined in this doc that's currently a work in progress: https://github.com/dequelabs/cauldron/blob/documentation/documentation/home.md
This would apply to text size classes as well as in the future, in my opinion. (e.g. --text-size-56
,--text-size-45
, etc.
Additionally, we should have more standardization in text size as these sizes are all over the place for no rhyme or reason, which is also outlined in the document I provided for future consideration.
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.
Frankly, these are bad naming conventions because it's pretty ambiguous what each variable means without having to remember or go look at documentation.
I don't disagree that the naming convention was a poor choice in the past, but right now we don't have the resources to allocate towards making these changes. I do believe there is a time and place to have this discussion, but unfortunately feel that right now isn't the time to be making these sort of changes.
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.
Could we have both the old and new variable names together and slowly migrate over to the new ones when we have time?
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 @anastasialanz's suggestion is spot-on. Let's deprecate "old" variables and create new ones with names that make sense.
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 disagree that the naming convention was a poor choice in the past, but right now we don't have the resources to allocate towards making these changes. I do believe there is a time and place to have this discussion, but unfortunately feel that right now isn't the time to be making these sort of changes.
I agree, this is definitely a long-term issue we need to think about, so I don't have a bone to pick if we are just arbitrarily choosing names right now and will have an effort in the future to change the var names. You guys are the ones to have to figure out what naming convention works today 😄
Preview branch generated at https://feat-new-variables.d1gko6en628vir.amplifyapp.com |
This PR adds new variables based from new UX pin.
\cc @bobbyomari