-
Notifications
You must be signed in to change notification settings - Fork 677
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
V5 final final #705
V5 final final #705
Conversation
- New color system - New support for grid - Updated scales - New hover animations - Min width based media queries
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 lot of nice changes to bring things up to date. I question the mixture of modern CSS things along with remnants of IE support though.
@@ -18,71 +18,8 @@ | |||
|
|||
*/ | |||
|
|||
.b--black { border-color: var(--black); } |
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's the reason for removing all the border colors?
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.
agreed – we use the border color classes often.
src/_border-radius.css
Outdated
@@ -51,12 +56,14 @@ | |||
.br-initial { border-radius: initial; } | |||
.br-unset { border-radius: unset; } | |||
|
|||
@media (--breakpoint-not-small) { | |||
@media (--breakpoint-small) { | |||
.br0-ns { border-radius: 0; } |
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 thought you were removing all -ns
styles
-m = medium | ||
-l = large | ||
|
||
*/ | ||
|
||
.flex { display: flex; } |
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.
moved to _display.css
@@ -18,32 +18,32 @@ | |||
n = none | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small | |||
-m = medium | |||
-l = large | |||
|
|||
*/ | |||
|
|||
|
|||
|
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 you should move the float clears here instead of removing them entirely?
@@ -5,7 +5,19 @@ | |||
|
|||
*/ | |||
|
|||
/* Responsive images! */ | |||
img { display: block; max-width: 100%; } |
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.
Do you think you should be changing a bare img
tag like this? This could have unintended side-effects because the selector is too greedy
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.
Agreed. I appreciate the principle of responsive by default, but this seems like overreach.
maybe it could be opt in? maybe .responsive-image
to match the ones below??
@@ -24,6 +24,14 @@ body { | |||
margin: 0; | |||
} | |||
|
|||
/** | |||
* Render the `main` element consistently in IE. |
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're adding all this grid stuff and aspect ratios, but still care about IE for this? Seems like you should just make a clean cutoff for the sake of consistency.
src/_text-transform.css
Outdated
@@ -13,7 +13,7 @@ | |||
n = none | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
missed an -s
font-size: 5rem; | ||
} | ||
|
||
|
||
/* Type Scale */ |
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.
Changing the type scale is the biggest breaking change in this PR, IMO. I hope this was well-considered. It makes sense though.
src/_visibility.css
Outdated
@media (--breakpoint-not-small) { | ||
.clip-ns { | ||
@media (--breakpoint-small) { | ||
.clip-s { | ||
position: fixed !important; | ||
_position: absolute !important; |
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.
The underscore hack died with IE6, there's no reason that I'm aware of to keep this.
@@ -51,23 +51,15 @@ | |||
.w-10 { width: 10%; } | |||
.w-20 { width: 20%; } | |||
.w-25 { width: 25%; } | |||
.w-30 { width: 30%; } | |||
.w-33 { width: 33%; } |
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.
Not a fan of removing thirds. At the Spokesman-Review, a ton of pages use three up layouts. The 30, 40, etc., are less important, but being able to have 33% is useful.
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.
agreed. we use it a bit too.
I love ALMOST everything but ... I think the switch from h,v to x,y is a mistake. Not only does it have a major impact on upgradability, it's largely a vanity change, without adding anything to usability (infact possibly removing some). Just saying, as an avid fan of tachyons. I'll likely have to fork the branch to put it back for my project uses. |
Separately, what about 'row-gap' and 'col-gap' ? We've implemented this in our repo as |
@@ -20,7 +21,7 @@ | |||
-pill = 9999px | |||
|
|||
Media Query Extensions: | |||
-ns = small | |||
-n = small |
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.
-s ?
- Breaks align-items, align-self, flexbox, justify-self and order to separate files.
src/_letter-spacing.css
Outdated
@@ -4,7 +4,7 @@ | |||
Docs: http://tachyons.io/docs/typography/tracking/ | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
-ns = small | |
-s = small |
y = vertical | ||
x = horizontal |
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.
h/v
are great! I don't understand the motivation to change this 😢
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.
PR to revert this notation change: #709
src/_typography.css
Outdated
@@ -4,27 +4,27 @@ | |||
http://tachyons.io/docs/typography/measure/ | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
-ns = small | |
-s = small |
src/_utilities.css
Outdated
@@ -3,7 +3,7 @@ | |||
UTILITIES | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
-ns = small | |
-s = small |
src/_utilities.css
Outdated
@@ -22,7 +22,7 @@ | |||
.mr-auto { margin-right: auto; } | |||
.ml-auto { margin-left: auto; } | |||
|
|||
@media (--breakpoint-not-small){ | |||
@media (--breakpoint-small){ | |||
.center-ns { |
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.
.center-ns { | |
.center-s { |
src/_white-space.css
Outdated
@@ -3,7 +3,7 @@ | |||
WHITE SPACE | |||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
-ns = small | |
-s = small |
src/_widths.css
Outdated
@@ -34,7 +34,7 @@ | |||
|
|||
|
|||
Media Query Extensions: | |||
-ns = not-small | |||
-ns = small |
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.
-ns = small | |
-s = small |
src/_widths.css
Outdated
.w-100 { width: 100%; } | ||
|
||
.w-third { width: calc(100% / 3); } | ||
.w-two-thirds { width: calc(100% / 1.5); } | ||
.w-auto { width: auto; } | ||
|
||
@media (--breakpoint-not-small) { | ||
@media (--breakpoint-small) { |
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.
lots of -ns
in this media query
to summarize other comments --
PR to fix these: #708 |
This branch still has |
I'd step up and be willing to fix this branch and also implement a darkmode. I already did this with my own "fork" of tachyons (darkmode and min-width-sizes). See it here: https://screenisland.com/assets/css/tachyons.css I checked out the branch and found a few things that need work:
Is there something else that needs attention? I often prefer to have a production ready single css file to drop in and forget about. Tailwinds toolchain is often too much. |
Easily change grid cell size and color with --grid-color and --grid-size
Still want to tweak a few things here around configuring and scale structure.
Want to keep things very composable, flexible, and overrideable at multiple levels without bloating the library too to much. With variables I believe there is a lot of room for fun animations
src/_variables.css
Outdated
--gradient-color-15: var(--gold-0); | ||
--gradient-color-16: var(--gold-5); | ||
--gradient-color-17: var(--yellow-0); | ||
--gradient-color-18: var(--yellow-5);; |
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.
The doubled semicolon needs to be removed.
--gradient-color-18: var(--yellow-5);; | |
--gradient-color-18: var(--yellow-5); |
src/_variables.css
Outdated
--shadow-border-width: 1px; | ||
--shadow-opacity: .25; | ||
|
||
// Opacity scale |
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 not a valid css comment (happens to me also way too often :D)
// Opacity scale | |
/* Opacity scale */ |
There is currently a conflict with the .br class it is used for border-right and for border-radius. |
really looking forward this update <3 |
v1-4 all had the goal of being as absolute light weight as possible. v5 adds a number of modules and a full color system
but still weighs in around 20kb gzipped. Given how much CSS has expanded over the last few years, this added file size I think is reasonable.
Major changes
TODO
These can likely be follow up minor bumps: