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

Add a Sass function to convert pixels to rems (resolves #67) #69

Merged
merged 3 commits into from
Feb 18, 2020

Conversation

greatislander
Copy link
Contributor

This pull request adds a function for converting pixels to rems as requested in #67. It uses the unitless example from CSS Tricks, so the following will both work:

body {
  font-size: rem(20);
}
body {
  font-size: rem(20px);
}

@tatianamac tatianamac requested a review from ovlb February 15, 2020 10:26
@tatianamac
Copy link
Collaborator

Thank you, @greatislander! This is so awesome and helpful.

@ovlb would love a second review here. I think we'll also want to open up an issue to ensure that we change all instances of px to rems using this function.

@greatislander
Copy link
Contributor Author

@tatianamac Glad to contribute! I’m also happy to take care of the follow-up issue you mentioned, but I figured I would wait to let you review this before jumping ahead to converting everything to use the function :)

Copy link
Collaborator

@ovlb ovlb left a comment

Choose a reason for hiding this comment

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

One tiny thing regarding naming, otherwise everything works fine. Thanks, @greatislander!

If you want, just push the other px -> function replacements to this PR and we skip creating a follow-up issue :)

assets/css/base.scss Outdated Show resolved Hide resolved
@greatislander
Copy link
Contributor Author

Sounds good @ovlb!

@greatislander
Copy link
Contributor Author

One other thought occurred to me — structurally, would there be some value in moving Sass functions and mixins into their own partials that are imported at the top of the base file?

Copy link
Collaborator

@ovlb ovlb left a comment

Choose a reason for hiding this comment

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

Great. Thanks :)

@ovlb
Copy link
Collaborator

ovlb commented Feb 16, 2020

One other thought occurred to me — structurally, would there be some value in moving Sass functions and mixins into their own partials that are imported at the top of the base file?

Yes, there definitely is. I started this work for the regular classes in #75 but skipped the Sass part, as I waited for this PR to get merged.
Any preference on how to do it? In other projects, I have a folder named _sass in which I collect such partials. But am open to suggestions :) If you want to do this, go ahead!

@greatislander
Copy link
Contributor Author

@ovlb I think having _mixins.scss and _functions.scss partials in that _sass folder would make sense. I can move the function into such a partial in this PR — if you prefer I could make a separate one for the mixins so as to keep this PR within scope.

@ovlb
Copy link
Collaborator

ovlb commented Feb 16, 2020

@greatislander That sounds perfect, thanks!

@greatislander
Copy link
Contributor Author

@ovlb Done!

@tatianamac tatianamac merged commit 01b0018 into selfdefined:master Feb 18, 2020
Copy link
Collaborator

@tatianamac tatianamac left a comment

Choose a reason for hiding this comment

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

Perfect!

@greatislander greatislander deleted the add/rem-function branch February 29, 2020 22:26
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.

None yet

3 participants