-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
each() loop w/ & behaves funny #3340
Comments
Detached rulesets also behave unexpectedly. @infixes: a, b;
/*! DETACHED RULESET */
.dr {
each(@infixes, #(@_infix, @i) {
@infix: e(if(
((@i + 1) <= length(@infixes)),
%('-%s', extract(@infixes, (@i + 1))),
''
));
@content();
});
}
@content: {
&@{infix} {
display: block;
.child { display: block; }
}
}; /*! DETACHED RULESET */
.dr {
display: block;
}
.dr-b {
display: block;
}
.dr-b .child {
display: block;
}
.dr .child {
display: block;
} |
Thanks @calvinjuarez. Do you think you could boil that down to the simplest example that demonstrates the bug? Or are |
@matthew-dean all those functions are just to say, "whatever the current item is, use the next one, unless this is the last one, in which case, use So in the loop, when So, to answer the question, no, they're not essential to trigger it (I expect it happens always), they're just a way of looping in the wonky way that exposes the issue. I ran into the issue while fixing the output order in seanCodes/bootstrap-less-port, where the same behavior is accomplished with I'll see if there's a less obtuse test case that produces a similar error. |
Here's something a tiny bit more concise, and hopefully easier to read. The key is that, in the CSS, when It looks like it has something to do with how @infixes: a, b;
/*! LOOP IN BLOCK */
.amp {
each(@infixes, #(@val, @i) {
@is-last: boolean((@i = length(@infixes)));
@infix: if(@is-last, ~'', ~'-@{val}');
&@{infix} { -less-original-value: @val; }
});
}
/*! LOOP IN BLOCK – CONTROL */
.control {
each(@infixes, #(@val, @i) {
@is-last: boolean((@i = length(@infixes)));
@infix: if(@is-last, ~'-', ~'-@{val}');
// ^ identical to previous except @infix is non-empty, even when @is-last is true
&@{infix} { -less-original-value: @val; }
});
} /*! LOOP IN BLOCK */
.amp {
-less-original-value: b;
}
.amp-a {
-less-original-value: a;
}
/*! LOOP IN BLOCK – CONTROL */
.control-a {
-less-original-value: a;
}
.control- {
-less-original-value: b;
} |
I'm getting the feeling this is expected behavior out of .control-2 {
each(@infixes, #(@val, @i) {
@infix: ~''; // @infix is always empty
&@{infix} { -less-original-value: @val; }
});
} .control-2 {
-less-original-value: a;
-less-original-value: b;
} That's probably expected. This looks like it'd be a hairy one to sort out...unless we're okay making this output as 2 rules with the same selector. :/ |
At quick glance I suppose .amp {
/* 0 */
a {/* 1 */}
& {/* 2 */}
b {/* 3 */}
} this should result in: .amp {
/* 0 */
/* 2 */
}
.amp a {/* 1 */}
.amp b {/* 3 */} And the same order is expected if you remove |
Yeah, I never really thought about placement order of nested blocks. But it definitely seems like Less doesn't try to keep things in order. To expand on @seven-phases-max's example: .amp {
is: one;
.a { is: two }
& { is: three }
.b { is: four }
is: five;
} Outputs: .amp {
is: one;
is: three;
is: five;
}
.amp .a {
is: two;
}
.amp .b {
is: four;
} Sass's output is slightly different, opting to keep nested selectors in order, even if one is just .amp {
is: one;
is: five;
}
.amp .a {
is: two;
}
.amp {
is: three;
}
.amp .b {
is: four;
} Oddly, Sass doesn't treat the So, if you're migrating Bootstrap and comparing output, you're likely seeing that Less collapses The As far as which behavior makes more sense? 🤷♂️ I really don't know how to answer that. They're just different. I think the rationale in Less's behavior is to create fewer selectors. But Sass's rationale is probably to keep nested selector order intact, but it probably comes down to the merging algorithms are just different. Now, I were to put my CSS hat on, and thinking about things like the .amp {
is: one;
}
.amp .a {
is: two;
}
.amp {
is: three;
}
.amp .b {
is: four;
}
.amp {
is: five;
} That is, declarations should maintain their order in the stylesheet, if they're broken up by other nested selectors. If nesting were ever added to CSS natively, that's likely what it would mean. It's more verbose, but it seems more accurate. |
Yeah, I think we're all seeing eye-to-eye. What we've got is reliable and expected in nearly all cases (the examples shared in the previous 2 comments, for instance). You kind of have to work to find how to "break" it. Even in those situations, though, the workaround doesn't suck, since Given all that, I'm closing this for now (won't fix, intended behavior). If we really wanted to allow forcing output the way sass does, it would need to be a new feature, and it'd need a SUPER compelling use case. And for posterity: WorkaroundRefactor so that .amp {
each(a b, #(@val, @i) {
@is-last: boolean((@i = length(@infixes)));
@infix: if(@is-last, ~'', ~'-@{val}');
&@{infix} {
-less-original-value: @val;
}
});
} BECOMES each(a b, #(@val, @i) {
@is-last: boolean((@i = length(@infixes)));
@infix: if(@is-last, ~'', ~'-@{val}');
.amp@{infix} {
-less-original-value: @val;
}
}); |
@calvinjuarez Btw, you can simplify your code slightly like this: @infixes: a b;
each(@infixes, #(@val, @i) {
@is-last: boolean(@i = length(@infixes));
@infix: if(not(@is-last), ~'-@{val}');
.amp@{infix} {
-less-original-value: @val;
}
}); That is, |
Interesting. I tried dropping the parens, but it didn't compile for me at the time. I'll double-check. The bootstrap-less-port handles that logic with plugin functions (to be more familiar to folks coming from the Sass stuff), but I'll have to keep that in mind in case we decide to go native-er with it at some point. |
* package – update css-compile script to use --math --strict-math has been deprecated and replaced by --math. See less/less.js#3274 * maps – begin converting to DRs * each – use each() for loops (WIP) Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates. * breakpoints – fix breakpoint JS after converting to DR maps * each – replace all #each-* loop with each or #for-* There are 4 loops in mixins/_grid-framework.less that loop 'til a static number. I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`. * dev, meta – steal nice stuff from #12 Thanks @matthew-dean! * plugins – fix lint warnings/errors Except one in breakpoints.js: ``` 91:6 warning Variable 'nextBreakpoint' should be initialized on declaration ``` * _tables – fix .table-responsive output order See less/less.js#3340 * _navbar – fix .navbar-expand output order See less/less.js#3340 * README – update to match true min version required Co-Authored-By: calvinjuarez <[email protected]> * plugins/theme-color-level – more readable code Co-Authored-By: calvinjuarez <[email protected]> * README – replace the other, less easy ways of installing * _grid-framework – restore comment See #14 (comment) * README – wording edits Co-Authored-By: calvinjuarez <[email protected]> * _functions – restore commented Sass * README – more wording edits Co-Authored-By: calvinjuarez <[email protected]> * README – remove clone note per #14 (comment) * plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403 * rename plugin `keys` → `map-keys` Reverting the name change for now, until @calvinjuarez is able to pull in the plugin from NPM and alias it. (See [his comment](#14 (comment) sion_r241965904).) This change addresses [this PR review comment](#14 (comment) sion_r239916331). * _grid-framework – revert loop renaming Reduce the number of changes in the PR to avoid potential bugs. This change addresses [this PR review comment](#14 (comment)). * _transition – revert var renaming Match var names used in the Sass version of the mixin. This change addresses [this PR review comment](#14 (comment)). * breakpoints – put long comments on own line (instead of including at the end of a line) * restore color-function plugins Revert deletion of plugin files that add the `color()`, `gray()` and `theme-color()` functions. Even though Less’ new ruleset accessors could be used, we’re going to keep them for parity with the Sass version and for backwards compatibility. This change addresses [this PR review comment](#14). * restore usages of color functions Revert the conversion the `color()`, `gray()` and `theme-color()` functions to Less’ ruleset accessors. This change is primarily being made to keep the syntax as similar to the Sass version as possible, for easy reference/comparison. Note that keeping things similar to Sass may prove unnecessary in the future, so we may go back to accessors, but for now we’ll keep it like this. This change addresses [this PR review comment](#14). * change color functions to handle rulesets vs lists * fix bug in ruleset-to-map conversion Instead of pulling the `value` directly from each item in the ruleset, evaluate the item first to determine the true value of the item. This fixes the issue where the `value` is actually a var name and what we want is the the value of the var—not the var name itself.
* package – update css-compile script to use --math --strict-math has been deprecated and replaced by --math. See less/less.js#3274 * maps – begin converting to DRs * each – use each() for loops (WIP) Still haven't updated for the grid breakpoints just yet, though some breakpoint loops have been written in anticipation of those updates. * breakpoints – fix breakpoint JS after converting to DR maps * each – replace all #each-* loop with each or #for-* There are 4 loops in mixins/_grid-framework.less that loop 'til a static number. I'm not aware of any answer for that at the moment, but a plugin could be easily written that could just leverage `each()`. * dev, meta – steal nice stuff from #12 Thanks @matthew-dean! * plugins – fix lint warnings/errors Except one in breakpoints.js: ``` 91:6 warning Variable 'nextBreakpoint' should be initialized on declaration ``` * _tables – fix .table-responsive output order See less/less.js#3340 * _navbar – fix .navbar-expand output order See less/less.js#3340 * README – update to match true min version required Co-Authored-By: calvinjuarez <[email protected]> * plugins/theme-color-level – more readable code Co-Authored-By: calvinjuarez <[email protected]> * README – replace the other, less easy ways of installing * _grid-framework – restore comment See #14 (comment) * README – wording edits Co-Authored-By: calvinjuarez <[email protected]> * _functions – restore commented Sass * README – more wording edits Co-Authored-By: calvinjuarez <[email protected]> * README – remove clone note per #14 (comment) * plugins/breakpoints – remove parseUnit(); fix nextBreakpoint ESLint issue See https://github.com/seanCodes/bootstrap-less-port/pull/14/files/94a03d156dde954caccf58130ebc8fa3adaf397e#r240023403 * rename plugin `keys` → `map-keys` Reverting the name change for now, until @calvinjuarez is able to pull in the plugin from NPM and alias it. (See [his comment](#14 (comment) sion_r241965904).) This change addresses [this PR review comment](#14 (comment) sion_r239916331). * _grid-framework – revert loop renaming Reduce the number of changes in the PR to avoid potential bugs. This change addresses [this PR review comment](#14 (comment)). * _transition – revert var renaming Match var names used in the Sass version of the mixin. This change addresses [this PR review comment](#14 (comment)). * breakpoints – put long comments on own line (instead of including at the end of a line) * restore color-function plugins Revert deletion of plugin files that add the `color()`, `gray()` and `theme-color()` functions. Even though Less’ new ruleset accessors could be used, we’re going to keep them for parity with the Sass version and for backwards compatibility. This change addresses [this PR review comment](#14). * restore usages of color functions Revert the conversion the `color()`, `gray()` and `theme-color()` functions to Less’ ruleset accessors. This change is primarily being made to keep the syntax as similar to the Sass version as possible, for easy reference/comparison. Note that keeping things similar to Sass may prove unnecessary in the future, so we may go back to accessors, but for now we’ll keep it like this. This change addresses [this PR review comment](#14). * change color functions to handle rulesets vs lists * fix bug in ruleset-to-map conversion Instead of pulling the `value` directly from each item in the ruleset, evaluate the item first to determine the true value of the item. This fixes the issue where the `value` is actually a var name and what we want is the the value of the var—not the var name itself.
Given the following, I would expect both loops to render similar output. However, the version where the
each()
is inside the block and&
is used produces strange results. Specifically, the styles immediately under the&
are rendered at the top of the loop's generated output. Oddly, a child of the&
block is rendered in the expected spot.Given
Expected
Actual
Note that
.amp
is floated to the top.The text was updated successfully, but these errors were encountered: