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

fix(styles): add error handling for rem & em sass functions #4931

Merged
merged 5 commits into from
Dec 23, 2019
Merged

fix(styles): add error handling for rem & em sass functions #4931

merged 5 commits into from
Dec 23, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Dec 20, 2019

Related to #4926 & #4927

I think it would be a good idea to add a little bit of error handling to the carbon--rem/carbon--em functions (and their deprecated versions too).

Since these functions all require pixel values to output valid CSS, it would be nice to notify developers directly from these functions when a non-pixel value is received. (Without pixel values, the functions return invalid output -- see this CodePen for examples: https://codepen.io/jendowns/pen/PowpyQJ)

Changelog

New

  • add @error @warn check to carbon--rem/rem and carbon--em/em functions

@jendowns jendowns requested a review from a team as a code owner December 20, 2019 17:09
@ghost ghost requested review from abbeyhrt and asudoh December 20, 2019 17:09
@netlify
Copy link

netlify bot commented Dec 20, 2019

Deploy preview for carbon-elements ready!

Built with commit e8cba9f

https://deploy-preview-4931--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Dec 20, 2019

Deploy preview for carbon-elements ready!

Built with commit 859a3ed

https://deploy-preview-4931--carbon-elements.netlify.com

@joshblack
Copy link
Contributor

What would you think of the following warning format:

Expected argument $px to be of type `px`, instead received: `#{unit($px)}`

@netlify
Copy link

netlify bot commented Dec 20, 2019

Deploy preview for carbon-components-react ready!

Built with commit 859a3ed

https://deploy-preview-4931--carbon-components-react.netlify.com

@joshblack
Copy link
Contributor

@jendowns also, would adding @error here be considered a breaking change? Would it be better to use @warn so the sass build doesn't fail? Also would understand if we want it to fail in this case, but curious if people would consider it a breaking change as a result

@netlify
Copy link

netlify bot commented Dec 20, 2019

Deploy preview for the-carbon-components ready!

Built with commit 859a3ed

https://deploy-preview-4931--the-carbon-components.netlify.com

@jendowns jendowns changed the title fix(styes): add error handling for rem & em sass functions fix(styles): add error handling for rem & em sass functions Dec 20, 2019
@jendowns
Copy link
Contributor Author

jendowns commented Dec 20, 2019

Thanks @joshblack! Just made the changes you suggested. Personally I prefer @error because I think failing, in this case, would help people find & address invalid values faster. BUT I can totally see how that would be breaking / disruptive so @warn seems like a safe way to go, just in case.

@joshblack
Copy link
Contributor

@jendowns agreed that @error is preferred here, for sure. Would we want to leave a comment like:

// TODO: update to @error in v11

Above the @warn blocks? That way we can keep track of this for our next change. We could also comment on this change over in #4358 as well!

@jendowns
Copy link
Contributor Author

Thanks @joshblack - I like that idea! I just pushed a commit with those comments added. I'll also add a comment to the issue you linked to, for v11 changes (unless you wanted to do that?)

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @jendowns!

@asudoh asudoh merged commit bbee3c4 into carbon-design-system:master Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants