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

Remove scss extensions from @material/mdc-* imports #62

Merged
merged 5 commits into from
May 24, 2018

Conversation

johnelliott
Copy link
Contributor

@johnelliott johnelliott commented May 22, 2018

Hi, I would like to help with material-components-web-react.

The sass imports are handy, but I was having trouble importing SCSS from Button. I needed to directly import the css from the non-react package, basically duplicating the intent of the index.scss of react-button:
image

When I looked at the icon component sass import I noticed that it didn't have an extension. Going into node_modules and removing it from the button index.scss file fixed the issue and allowed me to just import the button styles with @import "@material/button/mdc-button";, so perhaps this was just an oversight.

I've never used learn before, so I may need help figuring out how to contribute to this codebase :/

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@johnelliott
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@moog16
Copy link

moog16 commented May 22, 2018

@johnelliott Thanks for opening the PR. You're right it's not necessary and we can remove the extension from the filename. Can you also remove it from card, floating-label, line-ripple, and text-field Sass files for consistency?

Also something I realize is that you do not have the creds for the screenshot tests. I pushed a commit to see if it works for me.

@johnelliott
Copy link
Contributor Author

johnelliott commented May 22, 2018

@moog16 thanks Matt. I like this lib; want to show my support :)

I'll update too.

@johnelliott
Copy link
Contributor Author

Is it OK to add line-ripple, floating-label to the allowed scopes for commit messages?

@codecov-io
Copy link

codecov-io commented May 22, 2018

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   99.17%   99.17%           
=======================================
  Files          18       18           
  Lines         607      607           
  Branches       49       49           
=======================================
  Hits          602      602           
  Misses          5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 203cf34...209a6b0. Read the comment docs.

@johnelliott johnelliott changed the title Remove scss extension from button scss index Remove scss extension from @material/mdc-* imports May 22, 2018
@johnelliott johnelliott changed the title Remove scss extension from @material/mdc-* imports Remove scss extensions from @material/mdc-* imports May 22, 2018
@johnelliott
Copy link
Contributor Author

johnelliott commented May 22, 2018

Perhaps this test failure is also permissions-related for the screenshot job.

@moog16
Copy link

moog16 commented May 23, 2018

RE allowed scopes: YES definitely

RE Testing: Looking into Docker. Might be able to help us here.

@johnelliott
Copy link
Contributor Author

Ok, I use Docker and maybe that'd be the easiest way to get many people up and running those tests. I would be OK helping to set that up but I'd need initial direction.

Copy link

@moog16 moog16 left a comment

Choose a reason for hiding this comment

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

@johnelliott I think you're right, but I'm not sure why. Maybe its because you're out of the org? I will look into this. But I'll accept the changes.

@moog16
Copy link

moog16 commented May 24, 2018

Opened this issue concerning tests: #71. I'm just gonna run the tests locally and merge this for you. Thanks!

@johnelliott
Copy link
Contributor Author

Thanks @moog16

@moog16 moog16 merged commit 46b7f08 into material-components:master May 24, 2018
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.

4 participants