border radius mixin #46

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@chrisdl
chrisdl commented Jun 11, 2012

Changed border radius mixin to conform to the shorthand property used by border-radius and -moz-border-radius. (put topleft first instead of last)

@chrisdl chrisdl Changed border radius mixin to conform to the shorthand property used…
… by border-radius and -moz-border-radius. (put topleft first instead of last)
682dfb3
@jina jina commented on the diff Jun 12, 2012
less/mixins.less
@@ -73,19 +73,13 @@ border-radius : @radius; }
// .border-radius(VALUE,VALUE,VALUE,VALUE);
-.border-radius(@topright: 0, @bottomright: 0, @bottomleft: 0, @topleft: 0) {
+.border-radius(@topleft: 0, @topright: 0, @bottomright: 0, @bottomleft: 0) {
@jina
jina Jun 12, 2012 Contributor

Hi @chrisdl — is this change needed? The current ordering matches the ordering below, and the clockwise ordering that is commonly used in CSS, but I'm happy to hear your thoughts on why it should be changed. :)

@jina
jina Jun 12, 2012 Contributor

Oh, whoops, I misread this. It's actually in the right order. Guess the ones below should be reordered. :) My bad.

@drublic
drublic commented Jun 12, 2012

Changing the border-radius-mixin to take the four values according to the specs order makes sense here. +1 on including it.

The rest of the commit is discussable to my mind:
I'd suggest to write the webkit-version of border-radius as shortcut too as suggested with Mozilla's and the spec'd version.
I am not sure why the values should be separated by a comma. This is not a standard thing. Am I missing something here?

Apart from that I wonder if it's necessary to include the webkit- and moz-prefixed version of this property at all or if it could rater be dropped. border-radius in its unprefixed version is supported in every major browser on desktop and mobile. Only browser to slightly worry about should be Firefox 3.6.
I guess the support for this is a whole other discussion.

My suggestion for this PR is to fix the mixin's output before merging.

@jina
Contributor
jina commented Jun 12, 2012
  • Agreed on the reordering (makes total sense).
  • As for the shorthand values, that depends. I think the intent of this mixin is created to allow you to pass anything in here (maybe you have something totally different in each corner…), and this way seems to try to have better compatibility. But if we're not caring about certain browser versions (like Firefox 3.6), maybe that doesn't matter. @malarkey — thoughts?
  • Agreed that the commas are a bit weird.
@chrisdl
chrisdl commented Jun 13, 2012

You are correct, the commas are a mistake by me. Glad we got that established ;)

http://caniuse.com/border-radius

-moz- is good for 3.6 and older but it degrades fairly gracefully (or so I hear from html5please). And webkit it only really needed for about 5% of users (safari 4.0 and below).

I reckon, why not support it?

@drublic
drublic commented Jun 13, 2012

I guess you're right with "why not support them". It's mostly two loc and that's totally acceptable.
Apart from that combining the webkit-prefixed properties feels better for me.

@chrisdl
chrisdl commented Jun 13, 2012

I would combine for webkit but I got scared by this article on webkit for safari needing it specified in non shorthand: http://csswizardry.com/2010/03/a-quick-note-on-border-radius/

... thoughts?

@drublic
drublic commented Jun 13, 2012

Well, this seems to be an issue in Safari 4. I wouldn't care too much about this version. I mean there are not that much users for Safari 4 anymore (4.0: 0.22%, 4.1: 0.16% according to StatsCounter) and border-radius degradates gracefully.

@chrisdl
chrisdl commented Jun 13, 2012

Thanks for all the input. I've learn't a lot about border-radius throughout this discussion. I think I will let Malarkey decide how he wants the -webkit- prefixing to be done and just leave the pull-request as is (with the split -webkit-), but as drublic states a shorthand implementation of webkit would probably be fine.

@drublic
drublic commented Jun 25, 2012

Any more thoughts on this? @jina Wanna merge it in somehow?

@jina
Contributor
jina commented Jun 26, 2012

I want to see what @malarkey thinks on this before I make any changes, just in case he had a particular reason for doing it this way. :)

@chrisdl
chrisdl commented Jun 26, 2012

Makes sense @jina . I ended up removing the -webkit- prefix for border-radius in my development version, but I can't remember why for the life of me.

@chrisdl
chrisdl commented Mar 15, 2016
  1. guessing nothing will happen here. off to the graveyard with thee.
@chrisdl chrisdl closed this Mar 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment