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

Improved max/min functions, 2 new built in funtions. #1798

Merged
merged 9 commits into from
Feb 11, 2014
Merged

Improved max/min functions, 2 new built in funtions. #1798

merged 9 commits into from
Feb 11, 2014

Conversation

deviprsd
Copy link
Contributor

max/min improvements

  • Now, Numbers w/o units and numbers with units are compatible. //max(0, -2px) -> 0
  • First enter basis preference. max(0, -2px) //0 gets a px unit. (Which will not be reflected).

zero :

It takes a dimensional value (2px, 56%, 7em, ...) as parameter and
truncates 0unit to only 0 while rest values remain unaltered.

Example: zero(0em) // 0 | zero(2px) // 2px | zero(5%) // 5% | zero(0%) // 0

unitonly :

It takes a dimensional value (2px, 56%, 7em, ...) as parameter and
returns the unit of the value passed.

Example: unitonly(2px) // px | unitonly(2em) // em | unitonly(2%) // %

Grunt Test

Failed 1, Passed 101
max("junk", 5) -> 5; Should have resulted in max("junk", 5)

I personally feel strings should not be accepted it they are not being checked. I have plans for string support. Find max of their lengths. But not now.

max(0, 1em, 2, 4px) //returns 4px on the basis of first enter basis. Previous calculation would result in max(2, 1em, 4px). Here, 2 and 1em is compared, 2 is returned. then 2 and 4px is compared. Resulting in 4px. max(0, 1em, 1, 4px) //max(1em, 4px)
max(0, 1em, 2, 4px) //returns max(2, 4px) on the basis of first enter basis, as 2 gets em unit. Now em and px are incompatible till latest version of LESS.
max(0, 1em, 2, 4px) //returns max(2, 4px) on the basis of first enter basis, as 2 gets em unit. Now em and px are incompatible till latest version of LESS.
@deviprsd
Copy link
Contributor Author

max / min function are a part of css3, Since, it is not yet supported by any major browser. Till then LESS should support it.

@lukeapage
Copy link
Member

I like the min/max changes.

whats the use-case of the zero and unitonly functions? I can't think when I'd use them.. the compress option already removes units on zero dimensions, so it would only effect non compressed mode

@seven-phases-max
Copy link
Member

@lukeapage There is a history behind this PR: #1793. And the whole thing was inspired by this bootstrap use-case: twbs/bootstrap#12140.

@lukeapage
Copy link
Member

but as in those links, those 2 functions were needed to work-around the fixes to min max ?

I'm particularly against zero - I don't understand when that would be useful?

@seven-phases-max
Copy link
Member

but as in those links, those 2 functions were needed to work-around the fixes to min max ?

Yep. (Though I would not mind against "unitonly" with a better name as a method to get number's unit, but I can't actually imagine any interesting use-cases for it at the moment...).

@extemporalgenome
Copy link
Contributor

Then although I'm not in charge of anything either, for my part, a definite -1 on the idea of breaking forwards compatibility when min/max already have the property of forwards compatibility. Also, this code seems extremely verbose for the minor task it's actually trying to achieve (e.g. adding 11 lines of code just to break said forward compatibility).

@deviprsd
Copy link
Contributor Author

I didn't know about the compress property. In that case, zero gets a -1 from my side also. @seven-phases-max , I can't think of any better name for unitonly. You guys have any suggestions. There can be a lot of uses of unitonly.

@extemporalgenome as you suggested, i removed the handlers(check recent commits). So there, is no breaking of forward compatibility. But, if i don't add some error handler, errors like this can occur max(0, -2px, 1em) //max(0, 1em). On the first enter basis 0 gets a px.unit so it is not compared to em (px to em conversion not yet supported in LESS). max(0, 1em) will be misunderstood by the browser as a css3 function. Though, it is not yet supported i'm trying to add a single line handler. It will break the forward compatibility, so what, no browser supports max/min functions of css3. Whenever, support has been added, remove the handlers. Till then LESS cannot output max(args), which will not be validated by any browser.

@seven-phases-max
Copy link
Member

@deviprsd21 Honestly, I have no idea. Ideally it would be a ... unit but this name is already taken. So far I can't suggest anything that I like myself (getunit, get-unit or even extract-unit all seems to be weird for me).

@deviprsd
Copy link
Contributor Author

@seven-phases-max i first used getunit, totally weird to how other functions have been named in LESS.

@deviprsd
Copy link
Contributor Author

@extemporalgenome http://www.w3.org/TR/css3-values/ tell me where has max and min functions have been mentioned. It is official site of css3.

@deviprsd
Copy link
Contributor Author

Reduced 11 lines of code to 3 lines or 15 comparisions to 2... that sounds less verbose.

New Features for min/max

  • Error handlers for handling incompatible types.

@extemporalgenome
Copy link
Contributor

Agreed -- much less verbose now. You're right, @deviprsd21: all mention of min/max has disappeared from the spec. Apparently the Mozilla team vetoed it because, although they thought it was a good idea, design oversights in their code made it very difficult to implement. http://www.w3.org/blog/CSS/2009/07/30/resolutions_73/ shows the initial inclusion.

Since there's clearly no forward compatibility concern, error handling of incompatible units does indeed sound reasonable. Also, the zero-value case is just a subset of a broader behavior: negative values are always strictly less than zero values, and zero values are always strictly less than positive values. For example, min(-4px, 8em) should return -4px

@seven-phases-max
Copy link
Member

I have plans for string support. Find max of their lengths.

-1. For me looks like a sort of overengineering. Considering that the only "string" citizens of the CSS are URLs and content I would wonder what use-cases for a text/string processing we can imagine in that (CSS) context ... (It's pretty trivial to map some useful text/string processing functions to LESS, but what for?).

@deviprsd
Copy link
Contributor Author

@seven-phases-max i meant if ever someone wants string to be processed in max and min then how to compare them, length. For me comparing strings seemed silly, so i removed the addition of non-dimensional inputs into the array.

@deviprsd
Copy link
Contributor Author

@extemporalgenome I agree, with the problem you have pointed out. I'm on it. Tasks for me.

  • Remove zero, seems useless.
  • Find better name for unitonly.
  • Find fix for @extemporalgenome, that should be a part of broad behavior.
  • Comparing strings are out of question for now, I agree with the lexicographic order as suggested by @extemporalgenome

@extemporalgenome
Copy link
Contributor

I agree with @seven-phases-max -- if we can find a definite use case for string comparison, then it'd be worth looking into, but until then, knowing the particular implementation that is there for min/max, it's really only designed to conceptually deal with numerics/dimensions and leave everything else untouched -- it'd be easy enough to treat non-numerics as an error, but adding specific string support might be a lot of effort for what may turn out to be little gain.

I'm happy enough to see where this PR ends up -- if it gets that broader behavior, then I'll stamp my +1 on it.

@deviprsd
Copy link
Contributor Author

@extemporalgenome, it seemed silly to me at first and a lot of code, so, i removed there entry. I wanted a string support, therefore i mentioned my opinion about it in my PR. We need to discuss a lot how strings should be compared. There are so many comparisons, for that we need to find the most suitable one. Lexicographic order seems a great comparison, but at the other point it doesnt have a link with names max, and min. Therefore, i was thinking of using length.

@extemporalgenome
Copy link
Contributor

@deviprsd21 lexicographic comparison made sense to me because it's the standard string comparison in most languages and systems when dealing with strings. Beyond that, I've been thinking about min/max in a very technical sense -- the result of min is best described as the input that is less than all other inputs, thus if strings were supported in min/max, min('cde', 'abc', 'bcd') would return 'abc'. Others have raised good points though, that the only real quoted strings in CSS are URLs (well, and quoted font names), and there probably aren't good use cases in LESS for needed to get the least or greatest valued from amongst those. Even if we extend the concept to unquoted names, such as cursor names, color names, or font names (such as Arial), there probably aren't useful situations for comparing those, though it's entirely possible that we're all missing some key usecase.

@deviprsd
Copy link
Contributor Author

We are missing many things, @extemporalgenome the link i send you, http://www.w3.org/TR/css3-values/#absolute-lengths, it is mentioned, that 1 px is 1/96th of an inch so there are possible chances of conversion of px to m.

@extemporalgenome
Copy link
Contributor

@deviprsd21 sure, and that's good, though there are still many lengths that would be incomparable: the em family of units has no relationship to the other length unit families, nor do the vw/vh family relate comparably to others or even to the vmin/vmax sub-family; though of course all of these all still apply to the broad relationships mentioned prior. tl;dr we'll never be able to compare all unit combinations in all situations.

Also, something in the LESS core should be able to handle conversion of px <-> absolute length units -- within minmax and other functions, it should be as easy as calling a single API function/method that does the conversion for you, rather than each function needing to know the conversion ratio.

@deviprsd
Copy link
Contributor Author

@extemporalgenome true, but we can atleast try to broaden some. Maybe with time we will be able to compare all units. And em and % can be compared. 1em == 100% 1.2em == 120%, http://www.w3.org/TR/css3-values/#em-unit

@seven-phases-max
Copy link
Member

1em == 100% 1.2em == 120%

Of course not. Sure 1em is 100% of the current font size but this does not mean that width: max(2em, 100%); should result in width: 2em;

@deviprsd
Copy link
Contributor Author

@seven-phases-max / @extemporalgenome we need to fix some things before fixing max / min.

  • Addition of possible conversions, solves most problems of max and min
  • Getting all the arguments flattened before passing them to _minmax:
  • Strict rules based on @extemporalgenome suggestion, min(-4px, 2em)// -4px
  • And after all this is achieved, addition of string supprot.

@deviprsd
Copy link
Contributor Author

@seven-phases-max right, that didn't strike me, Is there any possibility in less that when a function is called we get the css property also.

@seven-phases-max
Copy link
Member

Is there any possibility in less that when a function is called we get the css property also.

We can't get a font-size property of interest because we don't know what HTML element the current ruleset will inherit properties from. Less has no HTML DOM knowledge (that's why browser can convert between px, em, % etc. but Less can't).

@deviprsd
Copy link
Contributor Author

seems em conversion is not possible for now. We should concentrate on px to absolute length conversion.

@deviprsd
Copy link
Contributor Author

Certain Fixes

@list: 1, 2, 3;
v3: max(@list);      // Previously Not OK
v3 revisited: max(@list); //3 // OK  

@deviprsd
Copy link
Contributor Author

Note: This PR contains bugs.

@deviprsd
Copy link
Contributor Author

@extemporalgenome Need help, implementing the rules you suggested. Help me !. Loads of code.

@extemporalgenome
Copy link
Contributor

@deviprsd21 which aspects in particular are proving difficult?

max(0, 1cm)// 0 ... 0 assumed unit "m". on comparison 0.01m was less than 0
@deviprsd
Copy link
Contributor Author

@extemporalgenome Latest commit fixes bugs of my promised broad behavior implement.

  • Now, Numbers w/o units and numbers with units are compatible. //max(0, -2px) -> 0
  • First enter basis preference. max(0, -2px) //0 gets a px unit. (Which will not be reflected). Took Wrong units

I want suggestions on how to achieve what you had suggested. That is proving difficult.

Updated Features

  • Less variables containing more than one value are now accepted by min, max. //@arr : 2,4,1,6; max(@arr); //6

@deviprsd
Copy link
Contributor Author

@extemporalgenome i want you to implement your idea into min and max. I'm out of time. Please do it. Then send a pull request to my less.js repository, so, that all the features remain in one single request.

@matthew-dean
Copy link
Member

min / max for string lengths? -1 That confuses its intended usage, which, as noted, is numerical.

@extemporalgenome
Copy link
Contributor

First enter basis preference. max(0, -2px) //0 gets a px unit. (Which will not be reflected). Took Wrong units

From my perspective, this should not result in 0px. It should just result in 0.

@extemporalgenome
Copy link
Contributor

@extemporalgenome i want you to implement your idea into min and max.

Which idea is that?

@deviprsd
Copy link
Contributor Author

@extemporalgenome

Also, the zero-value case is just a subset of a broader behavior: negative values are always strictly less than zero values, and zero values are always strictly less than positive values. For example, min(-4px, 8em) should return -4px.

This one and after this we will add string support with lexicographic comparison.

From my perspective, this should not result in 0px. It should just result in 0.

It results in 0, but according to first enter basis, if, max(0, 3mm, 4m) //0 gets a mm unit// and rest all numbers without unit will be treated as mm. If reversed, max(0, 4m, 3mm, 9) //0 and rest other numbers without unit get a m unit// none of the added unit will be reflected in the css.

@matthew-dean what is your opinion about lexicographic comparison for strings.

@deviprsd
Copy link
Contributor Author

@extemporalgenome are you on it, ... ??

@extemporalgenome
Copy link
Contributor

I'm not convinced that we need string support -- I was just saying that if we did have it, it should be lexicographic -- I wasn't saying we actually need it. Support for only dimension types will also simplify the implementation.

If I work on this, what will happen is pretty much a rewrite, since several existing properties are no longer needed, and a significant portion of the existing code is concerned with these properties. The properties are: 1) pass-through of non-dimension types, and 2) special care to make sure that unresolvable values remain in input order.

The new behavior would be: anything non-dimensional or unresolvable would result in a LESS error.

Because it would be a rewrite, there unfortunately wouldn't be benefit to basing off your existing work or piggybacking on your PR.

@deviprsd
Copy link
Contributor Author

@extemporalgenome that is what i tried to implement. I have removed property no 1. Pass through of non-dimensional elements are not allowed. Though no error is thrown. As, property 1 has been removed, there was no need of unresolvable values to remain in order, my modification doesn't care about the order.

And, about the string support, we had discussion, considering it, i thought of adding it, so what is your final opinion, string support or not ?

So, if ur answer is no, thats it. This PR is completely bugfree and ready for a merge.

@lukeapage
Copy link
Member

Hi,

I think obtain is a worse name than unitonly. I think getunit is a better name? thoughts anyone?

I'll merge this into 1.7 unless anyone else has any other objections?

@seven-phases-max
Copy link
Member

I think getunit is not bad after all (though get-unit would be my favourite from those mentioned above since it does not reads like G-tune-it :)

@extemporalgenome
Copy link
Contributor

”suffix”?
On Feb 11, 2014 7:39 AM, "Max Mikhailov" notifications@github.com wrote:

I think getunit is not bad after all (though get-unit would be my
favourite from those mentioned above since it does not reads like
G-tune-it :)


Reply to this email directly or view it on GitHubhttps://github.com//pull/1798#issuecomment-34759788
.

@lukeapage
Copy link
Member

get-unit is fine by me. suffix I think is misleading - its a unit. If unit(@A) didn't already have a meaning I'd use that

@lukeapage lukeapage merged commit 8ccadb8 into less:master Feb 11, 2014
@lukeapage
Copy link
Member

I have merged it in and changed the function name. I also added tests and fixed the tests including adding tests for a new bug exposed by this..

.bug-100cm-1m(@a) when (@a = 1) {
  .failed {
    one-hundred: not-equal-to-1;
  }
}
.bug-100cm-1m(100cm);

@deviprsd
Copy link
Contributor Author

@lukeapage thanks, i'll keep finding better solutions for mix and max functions, more work is required on it...

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.

5 participants