Refactor using native each(), bump Less version #12

matthew-dean · 2018-11-28T02:20:21Z

Hey Sean, took some time today to make this port a little cleaner using the new each() function. Let me know if you have any questions.

seanCodes · 2018-11-28T19:48:31Z

@matthew-dean Thanks for this! I'll take a look later today.

calvinjuarez

@seanCodes I think this is clean, except for the browser list discrepancy, which shouldn't be a big issue. In terms of whether to accept this one or #9, it's up to you. There are some fixes in this PR that I'd want to move to #9 before we merge it, though.

The only real difference is that I've migrated the 2-D lists to detached rulesets and used Less 3.5 property accessors, where @matthew-dean has opted to retain your more Sass-like map plugin functions. (See https://github.com/seanCodes/bootstrap-less-port/pull/12/files#diff-04c6e90faac2675aa89e2176d2eec7d8R111).

calvinjuarez · 2018-11-28T22:35:54Z

package.json

 		"css-prefix": "postcss --config build/postcss.config.js --replace \"dist/css/*.css\"",
 		"css-minify": "cleancss --level 1 --format keep-breaks --source-map --source-map-inline-sources --output dist/css/bootstrap.min.css dist/css/bootstrap.css && cleancss --level 1 --format keep-breaks --source-map --source-map-inline-sources --output dist/css/bootstrap-grid.min.css dist/css/bootstrap-grid.css && cleancss --level 1 --format keep-breaks --source-map --source-map-inline-sources --output dist/css/bootstrap-reboot.min.css dist/css/bootstrap-reboot.css",
 		"lint": "eslint ."
 	},
+	"peerDependencies": {
+		"less": "^3.7.1"


Suggested change

"less": "^3.7.1"

"less": "^3.8.1"

Should we match the peerDependency to the devDependency?

3.7 is the minimum required, so it will issue a warning to people who install this library. The dev dependency is just for tests, so it can be latest.

calvinjuarez · 2018-11-28T22:41:24Z

README.md


-0. **Loops** Sass `@for` and `@each` loops have been replaced with Less’s method of looping which requires unique, named mixins for every loop. This is a bit clunky and means that the loops used in this port are verbose and difficult to read, but it’s the best we’ve got until I can figure out how to overcome this with a plugin.
+0. **Loops** Where possible, Sass `@each` loops have been replaced by the Less `each()` function. Sass `@for` directives are trickier and have no direct Less analog (yet), so they are replaced with a method of looping which requires unique, named mixins for every loop. This is a bit clunky, and means that in some places, loops are verbose and difficult to read, but it’s the best we’ve got until I can figure out how to overcome this with a plugin or Less has a native equivalent.


Considering range() has a PR (less/less.js#3334), we could wait on this change and implement each()+range() to replace @for.

You certainly could!

BOOM. http://lesscss.org/functions/#list-functions-range

https://www.npmjs.com/package/less

calvinjuarez · 2018-11-28T22:44:44Z

dist/css/bootstrap-grid.css

  display: flex;
-  -ms-flex-wrap: wrap;
-      flex-wrap: wrap;
+  flex-wrap: wrap;


This change represents a deviation from Bootstrap's current compiled CSS (see https://github.com/twbs/bootstrap/blob/v4-dev/dist/css/bootstrap-grid.css#L66). Since one of the goals of this project is to deliver the same CSS as Bootstrap (or as much as possible), we should try to fix this. It's likely just a matter of updating the browser list settings.

Yes, that's entirely because of the browser settings for Autoprefixer. Although that would represent support as far back as IE10, which is barely used. However it's what Bootstrap v4 has. See: https://github.com/twbs/bootstrap/blob/v4-dev/.browserslistrc

Really you should just use the same browser list file if you want the same output.

Another good thing to do would be to import Sass Bootstrap as a dependency and run a diff tool on the generated CSS from Sass and from this port, so you could verify it.

Btw, just another word on IE10 -> It has a smaller user base than IE8 / IE9. So to support it and not support IE8 / IE9 hardly makes any sense, so I dunno, I feel like it's a counter-intuitive setting.

calvinjuarez · 2018-11-28T23:00:10Z

README.md

@@ -124,9 +108,9 @@ This port attempts to mirror the source Sass files as closely as possible in ord

   Note: The plugins are included using the [`@plugin`](http://lesscss.org/features/#plugin-atrules-feature) at-rule instead of as arguments to the `lessc` CLI. This was intentionally done since most Less GUI compilers don’t allow you to customize the command-line arguments.

-0. **Maps** Less has no _native_ concept of maps, which are used extensively in the Bootstrap Sass files. They can be emulated, however, by using a comma-separated list of space-separated lists, which is what is done in this port.
+0. **Maps** Less has native maps as of Less 3.5, but map keys must follow rules of property names (e.g. keys can't start with a number). Sass maps don't require those rules, so in this port, maps are emulated by using a comma-separated list of space-separated lists.


@matthew-dean So wait, does this break in 3.8? → https://github.com/calvinjuarez/bootstrap-less-port/blob/less3.7/less/_variables.less#L23 It didn't seem to have trouble compiling when I first tried it. (I'll have to try it out.)

Update: that map's not currently used anywhere, so that variable would never need to be resolved. :/

Update again, the @sizes map is used (with each() and works fine).

Wait so is there still a question?

I guess no, just that Less maps don't really enforce any requirements on properties. @grays[900]; works just fine, for example.

https://gist.github.com/calvinjuarez/7da0db15c421afcca1fc53e382936cce

Interesting. I guess I assumed the parser followed CSS rules in that regard.

I've done a PR to Microsoft for VSCode, so it should no longer flag:

a) Numeric properties (since they're apparently allowed in Less)
b) Mixin / DR lookups

microsoft/vscode-css-languageservice#135

calvinjuarez · 2018-11-29T00:21:23Z

dist/css/bootstrap.css

@@ -1647,6 +1494,13 @@ pre code {
 .table-dark.table-hover tbody tr:hover {
  background-color: rgba(255, 255, 255, 0.075);
 }
+.table-responsive {


@matthew-dean Any idea why this is being moved? It's happening in mine as well. It's generated by the each() at https://github.com/seanCodes/bootstrap-less-port/pull/12/files#diff-c4fb9381184254ae43a8b5d8ea35a322R194.

(Happens on #9 as well. I meant to ask you about it.)

That's really peculiar. It must be some side effect of the each() implementation. Although looking at the function, nothing stands out. It creates an array of generated rulesets. So there must be a bug somewhere in Less around where rulesets are flattened?

Never mind, this has to be a bug in one of the @plugin functions. Because if you just turn this into straight mixins, you get expected output:

See: https://gist.github.com/matthew-dean/9add256671e64b2c1d285129adb33658

Ah, yeah, actually this being up here is more expected I think. The .table-responsive > .table-bordered {...} block below (see link) should probably be up at the top as well. @seanCodes: worth noting.

https://github.com/seanCodes/bootstrap-less-port/pull/12/files/8337dccea789ed6d6d2b8ff5b45737a3ac60877d#diff-6972cbb37f0ec48771217d4915ea7e6fR1552

So it seems Bootstrap's intent with that loop is explicitly to have each iteration of the loop generate the next breakpoint's rules, with the final breakpoint instead generating un-infixed rules. Digging further, it looks to be a bug in Less (see less/less.js#3340).

@matthew-dean

Thanks @matthew-dean!

@matthew-dean

Thanks @matthew-dean!

@matthew-dean

Thanks @matthew-dean!

@matthew-dean

Thanks @matthew-dean!

@matthew-dean

* 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.

@matthew-dean

* 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.

Refactor using native each(), bump Less version

8337dcc

This was referenced Nov 28, 2018

Less 3.8 #9

Closed

Adds range() function for lists less/less.js#3334

Merged

calvinjuarez reviewed Nov 28, 2018

View reviewed changes

calvinjuarez reviewed Nov 29, 2018

View reviewed changes

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Nov 29, 2018

dev, meta – steal nice stuff from seanCodes#12

1789667

Thanks @matthew-dean!

calvinjuarez mentioned this pull request Dec 6, 2018

Less 3.9 #13

Merged

matthew-dean closed this Dec 6, 2018

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Dec 6, 2018

dev, meta – steal nice stuff from seanCodes#12

9213bb6

Thanks @matthew-dean!

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Dec 6, 2018

dev, meta – steal nice stuff from seanCodes#12

288d0ec

Thanks @matthew-dean!

calvinjuarez added a commit to calvinjuarez/bootstrap-less-port that referenced this pull request Dec 7, 2018

dev, meta – steal nice stuff from seanCodes#12

b565efa

Thanks @matthew-dean!


		0. Loops Sass `@for` and `@each` loops have been replaced with Less’s method of looping which requires unique, named mixins for every loop. This is a bit clunky and means that the loops used in this port are verbose and difficult to read, but it’s the best we’ve got until I can figure out how to overcome this with a plugin.
		0. Loops Where possible, Sass `@each` loops have been replaced by the Less `each()` function. Sass `@for` directives are trickier and have no direct Less analog (yet), so they are replaced with a method of looping which requires unique, named mixins for every loop. This is a bit clunky, and means that in some places, loops are verbose and difficult to read, but it’s the best we’ve got until I can figure out how to overcome this with a plugin or Less has a native equivalent.

Refactor using native each(), bump Less version #12

Refactor using native each(), bump Less version #12

Uh oh!

Conversation

matthew-dean commented Nov 28, 2018

Uh oh!

seanCodes commented Nov 28, 2018

Uh oh!

calvinjuarez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinjuarez Nov 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

calvinjuarez Nov 29, 2018 •

edited

Loading