Skip to content
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

[0.2] Combine sides and sizes for rounded utilities, like we do with border widths #188

Merged
merged 10 commits into from
Nov 15, 2017

Conversation

adamwathan
Copy link
Member

@adamwathan adamwathan commented Nov 11, 2017

Closes #204

This PR reverts an API design decision that was made just prior to 0.1.0 back to how it used to be in the secret days.

It combines rounded sides and sizes into one utility, like this:

<div class="rounded-t-lg"></div>

Right now, to make only one side of an element round, you need to combine a size utility and a side utility:

<div class="rounded-lg rounded-t"></div>

This had the benefit of not repeating the size utilities for every side, which seemed like a win.

There's a few downsides that ultimately I don't think ended up being worth it though:

  1. You have to combine two utilities for something that's actually done with one CSS property; that seems a bit weird.

  2. The implementation is too "clever". The way it works is when you add rounded-t, it sets the bottom border radiuses back to 0. I don't like that a class designed to say "make the top corners round" actually just makes the bottom corners square. It should be simpler and more obvious.

  3. You can't make 3 out of 4 corners rounded because of the implementation in point Update defaultConfig.js #2.

  4. The implementation mentioned in point Update defaultConfig.js #2 leads to this annoying situation where if you want to change what side is rounded at a different breakpoint, you have to respecify the size.

    For example, this won't work as you expect:

    <div class="rounded-lg rounded-t md:rounded-l"></div>
    

    You'd think that would have top rounded corners by default, and left rounded corners on medium screens and up. But what actually happens, is that on medium screens and up, only the top left corner is rounded, because rounded-t sets bottom radiuses to 0 and rounded-l sets right radiuses to 0. Grim!

    So currently you need to do this:

    <div class="rounded-lg rounded-t md:rounded-lg md:rounded-l"></div>
    

With the changes in this PR, sides and sizes are combined into the same utility like they are with border widths. So the example above would be written like this:

<div class="rounded-t-lg md:rounded-t-none md:rounded-l-lg"></div>

...which is a lot more like how you'd write similar border width code.

You can also now have two sides with different border radiuses if you really desire:

<div class="rounded-t-lg rounded-b-sm"></div>

This approach comes with the downside that the filesize is now a bit bigger because there are more utilities, but I think the added flexibility, increased consistency, and more obvious implementation is worth it.

Breaking Changes

This is a breaking change so it wouldn't land until 0.2.0.

Breaks include:

  • Border radius size utilities now no longer affect the radius of individually specified sides, ie:

    <div class="rounded-lg rounded-t"></div>
    

    ...would have normal sized rounded top corners, not large.

    Instead, you'd write that like this now:

    <div class="rounded-t-lg"></div>
    
  • The borderRadius section of the default config file has changed slighty, moving none to the top of the modifiers list so you can more easily use it to reset border radius at a breakpoint while still overriding one of those corners with another size. End users would need to update their config files to match this for their experience to match what's in the documentation.

Questions

I have two things I'm still considering that I'd be interested in input on:

  1. Right now we target sides like top, left, bottom, or right. That's not really how it works in CSS; instead in CSS you target top-left, top-right, bottom-right, or bottom-left.

    Our current utilities try to serve as a useful abstraction on top of this but it can make it really tricky to get things right when you're trying to round 3 corners or 1 corner.

    For example, this is the only way to round just the bottom right corner:

    <div class="rounded-lg rounded-t-none rounded-l-none"></div>
    

    That seems pretty lame to me vs. something like this:

    <div class="rounded-br-lg"></div>
    

    In general I think rounding a pair of corners is probably still the most common use case though, so I'm inclined to keep it the way it is. I don't think I'd like to add all 4 sides and all 4 corners, because the CSS bloat would be significant.

    Would the developer experience be significantly worse if you had to write this to make the top of an element rounded?

    <div class="rounded-tr-lg rounded-tl-lg"></div>
    

    I feel like it's probably worse, but it's definitely the most powerful approach :/ Open to discussion on that.

  2. I think I hate our default none modifier, and I think I'd like to change it to 0 by default, even though we use a named scale for the other sizes.

    Is this scale weird, or is it fine?

    {
        // ...
        borderRadius: {
            '0': '0',
            default: '.25rem',
            'sm': '.125rem',
            'lg': '.5rem',
            'full': '9999px',
        }
    }
    

To Do Before Merging

  • Update rounded documentation

@reinink
Copy link
Member

reinink commented Nov 11, 2017

I think I'd actually prefer this:

<div class="rounded-tr-lg rounded-tl-lg"></div>

Yes, it's longer than just using rounded-t, but I've always felt like reference to corners at "top" only was kinda weird.

As for the rounded-none vs rounded-0, I'm kinda indifferent. Does seems kinda weird to use "0" when the rest of the scale is named. What about rounded-square? :feelsgood: Just kidding. Hard problem. I almost want rounded-disabled or rounded-off. Kinda like rounded-off actually.

@ghost
Copy link

ghost commented Nov 12, 2017

I almost want rounded-disabled or rounded-off. Kinda like rounded-off actually.

I like the rounded-disabled better. The class rounded-off sounds like you are rounding something, which is the opposite of what the class is trying to invoke. Kinda like rounding-off numbers.

Anyways, I love the fact that you guys are discussing ideas here between each other, feels more open to the community 😄

@adamwathan
Copy link
Member Author

Part of me wants to rename this utility altogether to just radius-{modifier}, but it sort of kills the nice "no modifier" variant we use a lot now, since I think <div class="radius"></div> is a bit weird. Feels like there needs to be a value there.

But I do really like radius-0 or radius-none compared to rounded-0, rounded-none, rounded-disabled, etc.

If we did switch that name, I'm not sure what I'd want the default scale to be either.

radius-0
radius-sm
radius-md
radius-lg

Or:

// This one sort of sucks because right now we have a 0.125rem value for smallest
// radius, and that's not consistent with our numeric scale elsewhere (which is
// increments of .25rem)
radius-0
radius-1
radius-2
radius-4

...or who knows what else. Tough.

@mikebronner
Copy link

If I may provide an outside opinion for consideration, perhaps it helps when thinking about naming css classes (and perhaps not). :)

When naming classes, I think it is important to keep the naming to convey meaning and describe what the element looks like after it is applied. Focussing on the specific CSS property the rule pertains to seems to steer the "discussion" back to the technical side of things.

The class names become adjectives of the element they are applied to. This is in-line with most of the other naming conventions in Tailwind, like the colors, etc.

Because of that, I think rounded is still the better way to go.

@reinink reinink force-pushed the combined-rounded-api branch from 7c0bcf3 to d4f7b3b Compare November 13, 2017 21:47
@adamwathan adamwathan merged commit a94dcc5 into develop Nov 15, 2017
@adamwathan adamwathan deleted the combined-rounded-api branch November 23, 2017 13:49
DCzajkowski pushed a commit to DCzajkowski/tailwindcss that referenced this pull request Jul 23, 2019
Use vh rather than vw units in custom config example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants