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

Create abstraction for safer math.div usage #9857

Closed
tay1orjones opened this issue Oct 13, 2021 · 3 comments
Closed

Create abstraction for safer math.div usage #9857

tay1orjones opened this issue Oct 13, 2021 · 3 comments

Comments

@tay1orjones
Copy link
Member

Pointed out in #9850, math.div is only available in sass >=1.33.0. The Sass module system was introduced in 1.23.0. So it's possible for projects to be using sass modules, but still not have math.div available.

For these cases, it makes sense to wrap math.div usage in a conditional, using math.div when it's available, otherwise using legacy division:

@if meta.function-exists('div', 'math') {
  @return math.div($px, $base-font-size) * 1rem;
} @else {
  @return ($px / $base-font-size) * 1rem;
}

Unfortunately there is not a clean way to ensure this conditional is always used moving forward. Linting has a shortfall - the stylelint-scss plugin we use for stylelint rejected the proposal of a rule for no-division.

One way we could ensure correct usage would be to:

  1. Abstract the conditional out into its own helper function, div(dividend, divisor)
    • This helper should effectively replace all usages of math.div and legacy division.
    • math.div is used across various different packages in the monorepo. To facilitate reuse, the helper needs to be published as it's own new package in the monorepo.
  2. Ensure math.div is never used directly by disallowing it via Stylelint's function-disallowed-list rule.
    • The new helper function/package would need to disable/ignore this rule.
    • Ideally a custom error message would be displayed when someone tries to use math.div
      • Using math.div is not allowed, please use div() from @carbon/packageName instead
@tay1orjones
Copy link
Member Author

This comment patternfly/patternfly#4096 (comment) is essentially what we'd be aiming for.

I'm not sure if there's a difference between map.has-key(meta.module-functions("math"), "div") and
meta.function-exists('div', 'math')

@joshblack
Copy link
Contributor

@tay1orjones could we list sass@>=1.33.0 as a requirement for v11? I think we could list that version range as a peer dependency unless there is something else at play here 👀

@tay1orjones
Copy link
Member Author

@joshblack Yep, that would be my preference. It sounds like you agree, so I'll close this.

For now we'll stick with the conditionals that were added in #9850. In v11 we can remove them.

kennylam pushed a commit to kennylam/carbon that referenced this issue Jul 30, 2024
…-design-system#9857)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [karma-spec-reporter](https://togithub.com/tmcgee123/karma-spec-reporter) | [`0.0.35` -> `0.0.36`](https://renovatebot.com/diffs/npm/karma-spec-reporter/0.0.35/0.0.36) | [![age](https://badges.renovateapi.com/packages/npm/karma-spec-reporter/0.0.36/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/karma-spec-reporter/0.0.36/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/karma-spec-reporter/0.0.36/compatibility-slim/0.0.35)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/karma-spec-reporter/0.0.36/confidence-slim/0.0.35)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>tmcgee123/karma-spec-reporter</summary>

### [`v0.0.36`](https://togithub.com/tmcgee123/karma-spec-reporter/compare/v0.0.35...v0.0.36)

[Compare Source](https://togithub.com/tmcgee123/karma-spec-reporter/compare/v0.0.35...v0.0.36)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "every weekend" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Never, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/carbon-design-system/carbon-for-ibm-dotcom).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MS4wIiwidXBkYXRlZEluVmVyIjoiMzQuNzEuMCJ9-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants