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

Docs v5: Fix Sass rounding precision and state Sass implementation #32512

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

voltaek
Copy link
Contributor

@voltaek voltaek commented Dec 17, 2020

The switch from Node Sass to Dart Sass has ended up with all the output CSS having Dart Sass's fixed precision of 10. As mentioned in the current docs and also here, the intended final precision is 6, not 10. As such I've adjusted the parameters of both npm minification scripts to round to a precision of 6.

I also updated the site doc's blurb about precision values used with SASS building to reflect the current precisions used.

Copy link
Member

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was intentional, though. Please just keep the doc change to read 10 instead of 6.

@voltaek
Copy link
Contributor Author

voltaek commented Dec 17, 2020

@XhmikosR The only discussion on the topic I could find (linked in previous comment) made it seem like the intention was to still lower the precision to 6, at least for minified CSS. Is the precision to always be left at 10, then? If so I'll adjust the doc to read such. Do you recall the reasoning why we're not utilizing the free reduction in characters, so that I can properly reword the doc explanation?

@XhmikosR
Copy link
Member

#31728

We din't want to change the default output precision, at least not yet. Please only update the docs.

@voltaek
Copy link
Contributor Author

voltaek commented Dec 17, 2020

I've reverted the minification script changes and updated the doc to reflect the current build setup.

@voltaek voltaek changed the title Fix minified CSS rounding precision back at 6 Update Sass rounding precision in site doc Dec 17, 2020
@voltaek
Copy link
Contributor Author

voltaek commented Dec 17, 2020

I've expanded the explanation and added a mention of LibSass being deprecated

@mdo
Copy link
Member

mdo commented Dec 18, 2020

This feels like an odd thing to put here in our customization section. I'm not entirely sure why there's anything there about the precision even now. I don't feel like this is something really worth documenting honestly?

@XhmikosR
Copy link
Member

@mdo the current sentence is wrong right now:

In our build we've increased the Sass rounding precision to 6 (by default it's 5) to prevent issues with browser rounding.

@mdo
Copy link
Member

mdo commented Dec 18, 2020

Yeah I'd just rip all that out. Doesn't make sense to mention it at all tbh.

@XhmikosR
Copy link
Member

Your call, feel free to edit this PR or make a new one then.

BTW we do have an issue about mentioning the Sass implementation we support. #29760

@XhmikosR
Copy link
Member

For what is worth, I personally don't mind keeping this PR as is. It explains why people shouldn't lower the precision since we have been historically bit by that.

@voltaek
Copy link
Contributor Author

voltaek commented Dec 18, 2020

The Sass implementation the framework is built with and the precision used when building and/or minifying the CSS are both required knowledge to setup a build script when customizing Bootstrap. It would be nice to not have to read through the npm scripts just to gather this knowledge as a user of the framework, and if I were a new user to the framework I think that Docs > Customize > Sass is a reasonable page to navigate to to look for such things.

Perhaps the placement within the page isn't ideal, but I think just giving it a heading of "Building" would help clarify what the paragraphs are about and further help a new user find what they're looking for quickly and where they might expect the information to live.

Edit: I've added the heading and I think the placement of the paragraphs makes more sense now. Also the info is now even more easily found.

@voltaek
Copy link
Contributor Author

voltaek commented Dec 21, 2020

@XhmikosR Would you like me to create a similar PR for v4 that states the Sass implementation used in order to help close out #29760? If so, should that one also note that Node Sass is deprecated, or just state that its the implementation used and leave it at that (since it wasn't deprecated when v4 was created)?

@voltaek voltaek changed the title Update Sass rounding precision in site doc Docs: Fix v5 Sass rounding precision Dec 22, 2020
@voltaek voltaek force-pushed the sass-precision-updates branch 2 times, most recently from 976e87c to d787c98 Compare December 30, 2020 13:24
@voltaek voltaek changed the title Docs: Fix v5 Sass rounding precision Docs v5: Fix Sass rounding precision and state Sass implementation Dec 30, 2020
@voltaek
Copy link
Contributor Author

voltaek commented Jan 5, 2021

@XhmikosR @mdo Any more changes you're looking for with this or is the current wording and additional heading sufficient?

@XhmikosR
Copy link
Member

XhmikosR commented Jan 6, 2021

@voltaek sorry for the delay, it's holidays + quarantine. We'll get to do it soon :)

…mentation used, mention current precision of 10 due to Dart Sass, and mention recommended minimum precision (value of 6 used in BS v4). Remove outdated Sass precision from Customize > Sass docs page.
… Sass compiler mentions so we can keep all pertinent Sass compiler information in just one location instead of spread through the docs, which makes it hard to find the details and/or keep them up-to-date.
@XhmikosR
Copy link
Member

I'm going to merge this @voltaek, can you please backport this to v4? Without dart-sass since we use Node sass there.

@XhmikosR XhmikosR added the v4 label Jan 13, 2021
@XhmikosR XhmikosR merged commit f7d3870 into twbs:main Jan 13, 2021
@voltaek voltaek deleted the sass-precision-updates branch January 18, 2021 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants