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

Replace Sass division with multiplication and custom divide() function #34571

Merged
merged 3 commits into from
Sep 8, 2021

Conversation

mdo
Copy link
Member

@mdo mdo commented Jul 25, 2021

Fixes #34353. Replaces #34386.

@XhmikosR I removed the RFS changes from that other PR for now. Want to take a stab at porting it to this branch?

  • Add RFS changes in separate commit

@mdo mdo requested a review from a team as a code owner July 25, 2021 17:28
@XhmikosR
Copy link
Member

@mdo it seems on v4-dev we are using rfs 8.x. I'll try to see if I can cut a new 8.x release with the Dart Sass fix and then push here any patches.

That being said, I recall we still had some issues with the rounding on main. Are those sorted? If not, I'd wait until we fix everything before backporting.

@mdo
Copy link
Member Author

mdo commented Jul 25, 2021

I think everything has been sorted? We may have lost 1-2 decimals but I haven't heard any rounding bugs since we shipped the last release.

ffoodd
ffoodd previously approved these changes Jul 26, 2021
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

💪👌

@XhmikosR
Copy link
Member

@mdo I created an upstream branch https://github.com/twbs/rfs/commits/v8.x. Feel free to backport the divide fix with a PR. Just keep in mind that in the upstream 8.x branch we don't test dart Sass.

@mdo
Copy link
Member Author

mdo commented Jul 26, 2021

Pulled the latest in from that branch, thanks @XhmikosR! Just rebased and force pushed. Should be good to go.

@arcanedev-maroc
Copy link

@mdo Is it ok using 0.5 instead of .5 ?

I think it's considered as a "good practice" and also we're used to see float numbers with a 0 before the dot 👀

@darrinmn9
Copy link

really sorry to nag/comment, is the only thing left on this fix to convert * .{decimal} -> * 0.{decimal} for these commits?

@mdo
Copy link
Member Author

mdo commented Aug 23, 2021

Stylistically, we don't include leading 0s in any of our CSS values, so no need to revisit those :).

@evanfuture
Copy link

Hi, just wondering if this will move ahead, it's been getting stale, and it seems like an important thing. I'll reference the above Merge request comment (from petrsiegl):

There is minor problem with scss compilation and bootstrap. V4 bootstrap uses division symbol / which is deprecated in sass and causes warnings. The fix should be available soon (#34571). Until it is merged, there will be unfortunately warnings on compilation. They can be silenced by sass --quiet flag, but that disables other warnings as well.

ffoodd
ffoodd previously approved these changes Sep 1, 2021
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM, what would be missing now?

@XhmikosR
Copy link
Member

XhmikosR commented Sep 1, 2021

@ffoodd This is NOT ready due to RFS,please do not approve things like that because I might then accidentally merge stuff :)

@ffoodd
Copy link
Member

ffoodd commented Sep 1, 2021

Latest comments involving RFS felt accurate and up-to-date :D What's missing? Shouldn't we convert this to draft?

@ffoodd ffoodd self-requested a review September 1, 2021 09:18
@XhmikosR XhmikosR marked this pull request as draft September 1, 2021 12:03
@XhmikosR
Copy link
Member

XhmikosR commented Sep 1, 2021

@ffoodd the backported version doesn't have the division fix. We need an upstream patch against the https://github.com/twbs/rfs/commits/v8.x branch.

@cdubz
Copy link

cdubz commented Sep 1, 2021

Just to be clear — in order to get this approved twbs/rfs#398 (essentially) needs to be backported to RFS v8.x? @ffoodd are you planning to do that work or does someone else need to jump in?

@XhmikosR
Copy link
Member

XhmikosR commented Sep 1, 2021

Someone needs to do the work and provide an upstream PR, because the patch cannot be cherrypicked unfortunately.

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.

9 participants