-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(repo): add check around math.div usage #9850
fix(repo): add check around math.div usage #9850
Conversation
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: bbd9950 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6168a9377e86100008507ccc 😎 Browse the preview: https://deploy-preview-9850--carbon-react-next.netlify.app/ |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 40aa5d6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6165d14aaf4e1d0008228de2 😎 Browse the preview: https://deploy-preview-9850--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 40aa5d6 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6165d14a5999350007763bc6 😎 Browse the preview: https://deploy-preview-9850--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: bbd9950 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/6168a93769b3c70007be8527 😎 Browse the preview: https://deploy-preview-9850--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: bbd9950 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/6168a9371f72ad0007d803b1 😎 Browse the preview: https://deploy-preview-9850--carbon-elements.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked to make sure all usages are wrapped in the conditional.
A huge caveat here is that anytime math.div
is used moving forward, we'll have to actively remember to wrap it in this conditional. It's likely to get missed and break the build for consumers using sass <1.33.0.
My first thought is to tackle this with linting. The stylelint-scss
plugin we use for stylelint unfortunately rejected the proposal of a rule for no-division.
This conditional could be abstracted out into its own function to effectively replace math.div
, but direct usages of math.div
would still need to be disallowed in favor of using the abstraction. I think this could be done utilizing Stylelint's function-disallowed-list
rule.
@tay1orjones I thought about this, too. The disallowed-list is a great idea. I can abstract this into a function. Where do you think the best place to put it would be? Create a new utility in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevinsperrine I think since we're using math.div across various different packages we'll need to make a new internal package in the monorepo to share it between all the other packages. No need to do that here though - I think let's merge this as-is and we can tackle a the abstraction through a new issue.
Thanks, @tay1orjones. FWIW, I started a little of this this morning, but then realized it was going to require a number of other updates, so I stopped. I appreciate you getting this in! /// Used for dividing with backwards compatibility
/// @access public
/// @param {String|Number} $dividend the number to be divided
/// @param {Number} $divisor the number to be divided by.
/// @return {Number}
/// @example scss
/// // Will use math.div if it exists or fallback to `/` if not.
/// width: safe-math-div(100%, 2);
/// @group functions
@function safe-math-div($dividend, $divisor) {
@if meta.function-exists('div', 'math') {
@return math.div($dividend, $divisor);
} @else {
@return ($dividend / $divisor);
}
} |
Closes #9851
Some build tools have older versions of sass that does not include math.div. This adds a wrapper around math.div to maintain backwards compatibility.
Changelog
New
Changed
/
, if not.* 0.5
when previously was dividing by twoRemoved
Testing / Reviewing
confirmed existing tests pass. There shouldn't be any usages of
math.div
that are not wrapped in@if meta.function-exists('div', 'math') {