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

two new built in functions #1793

Closed
wants to merge 5 commits into from
Closed

two new built in functions #1793

wants to merge 5 commits into from

Conversation

deviprsd
Copy link
Contributor

@deviprsd deviprsd commented Jan 9, 2014

Named zero and unitonly.

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%) // %

zero converts 0px, 0em, 0% to only 0.
unitonly returns the unit of the number.
@seven-phases-max
Copy link
Member

Wouldn't it make more sense to improve min/max functions rather then introduce two new functions only to overcome min/max limitations?
unitonly seems to be handy but its name needs further discussion I guess.

Either way here's a workaround for your particular use-case w/o introducing new functions:

.border-top-radius(@radius) {
    border-top-right-radius: max((0 * @radius), @radius);
    border-top-left-radius:  max((0 * @radius), @radius);
}

Or an alternative (less readable) workaround w/o using min/max at all:

.border-top-radius(@radius) {
    border-top-right-radius: ((abs(@radius) + @radius) / 2);
    border-top-left-radius:  ((abs(@radius) + @radius) / 2);
}

@deviprsd
Copy link
Contributor Author

deviprsd commented Jan 9, 2014

OK,.... suggest a name for unitonly. But i think zero will also come in handy, when there is possibility of a return of 0 and other values.

Now the workaround presented by you gives what i expect to get. But the one think I dont want is 0 unit. I want only 0. Most people will want. I can't use unit unit(((abs(@radius) + @radius) / 2)). It will remove the unit of 0 along with other values.

Professionals use 0 instead of 0px, 0em, 0% etc. If they don't know what is being returned. Amateurs might change the variables, and if 0px or etc. is passed zero will easily convert it to 0, while keeping other values unaltered. May be not in max or min, probably for some other functions in future. zero is truly debatable. Give it a second thought

While I have a better idea for zero. Why not a function accepting 2 values. One parameter will accept the number which will be the base of comparison. The other will take the value whose unit needs to be removed.

1st Parameter = 2nd Prameter == true -> Remove Unit

I Personally Think, keeping simple zero is better.

@seven-phases-max
Copy link
Member

Well, let's just wait for other opinions.
(My personal opinion is that zero is a too low level optimization function making sense only in context of this particular snippet. But... if an optimized output is so critical I would vote for the original @tepez solution to the specific BS issue. I would offer some tricks to make it less verbose but since those tricks involve some features never used in the Bootstrap before I doubt those will be accepted. Besides I would also prefer max(0, -2px) to return 0 rather than doing that via zero(max((0 * @radius), @radius)).)

@deviprsd
Copy link
Contributor Author

deviprsd commented Jan 9, 2014

I want the same too, it would make the function more flexible. Anyway, for now unitonly seems a valid feature. I'll keep the zero for future. I'm trying to improve max and min.

I would offer some tricks to make it less verbose but since those tricks involve some features.

And do specify the tricks in detail.

@seven-phases-max
Copy link
Member

And do specify the tricks in detail.

Like this for example (it is actually possible to optimize the whole border-*-radius mixin family more (almost to the death) using new 1.6.0 property-interp feature but this would be complete overkill and would make things non-portable by now).

@matthew-dean
Copy link
Member

What exactly is being proposed here? I don't understand this.

@deviprsd
Copy link
Contributor Author

deviprsd commented Jan 9, 2014

Improved built in min/max functions

  • Fixed indents in some functions.
  • Better error handling min/max(1px, 2em) returns incompatible errors. Incompatible units like px | em, em | % ..etc. gives incompatible error.
  • Now, Numbers w/o units and numbers with units are compatible. //max(0, -2px) -> 0

On my side and opinion, I would like all the 3 functions merged. But @seven-phases-max , your decision will be my order. Zero can be used for complex math calculations or to prevent Amateurs from using 0unit. if not required in the mixin. Give another thought tozero. And suggest a name for unitonly.

Better error handling, now max(0, -2px) returns 0 ... etc.
Better error handling, now max(0, -2px) returns 0 ... etc.
@deviprsd
Copy link
Contributor Author

min file has not been updated.

@seven-phases-max
Copy link
Member

@deviprsd21
There's some activity in #1369 that can possibly overlap with your changes, so let's just wait for it too and then we'll see how to fit all this the best way (No need to rush anything since the next LESS "new features" version is not going to be released too soon anyway).

Also note that I do not make any decisions (for instance to merge something or not), I try to actively participate by expressing opinions and thoughts but I prefer to leave the actual decision to be made by other LESS team members (for multiple reasons).

P.S. Speaking of your code changes: note that the changes are to be made in less/lib files, that's where the compiler code is (see https://github.com/less/less.js/blob/master/CONTRIBUTING.md#pull-requests).

@deviprsd
Copy link
Contributor Author

closing this for, adding PR's properly.

@deviprsd deviprsd closed this Jan 10, 2014
@deviprsd
Copy link
Contributor Author

One more thing @seven-phases-max, i'm much of a intermediate to grunt. How to invoke and submit my test files.

@seven-phases-max
Copy link
Member

For tests you just edit (or create new) files in less/test. For running tests using grunt see https://github.com/less/less.js/blob/master/README.md#contributing

@extemporalgenome
Copy link
Contributor

@deviprsd21 min(1px, 2em) is not currently, and never should be an error. min and max are first and foremost CSS functions, not specifically LESS functions. The LESS min/max implementations are specifically there to be compatible with CSS, but to simplify statically resolvable cases. Please see http://www.dreamdealer.nl/articles/css3_calc_min_and_max_functions.html for examples of what I mean. min/max as implemented in LESS were specifically designed to only produce an error if called with zero arguments.

@deviprsd
Copy link
Contributor Author

Ok, so ill remove ... the errors handlers. But, max and min are not yet supported by any browser. Till then, LESS has to support it.

@deviprsd
Copy link
Contributor Author

@extemporalgenome max and min are not yet supported by any browser. Till then LESS should do it. So, according to me, since LESS doesn't support the conversion of px to em or % or absolute lengths so i'll keep the error handlers intact. Whenever max and min have been supported.I'll be the person to revert them back the original design.

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.

4 participants