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

Pure CSS #278

Merged
merged 5 commits into from
May 31, 2019
Merged

Pure CSS #278

merged 5 commits into from
May 31, 2019

Conversation

chrism
Copy link
Contributor

@chrism chrism commented May 24, 2019

This PR converts all the SCSS to CSS and removes SASS and ember-cli-sass completely.

@chrism chrism mentioned this pull request May 24, 2019
@samselikoff
Copy link
Collaborator

Nice work! Looks like there's a lint failure in the Travis tests.

This looks like a backwards-compatible change, correct? Could you write up some upgrade instructions (even in this PR) so they can be easily added when the next release rolls around?

@chrism
Copy link
Contributor Author

chrism commented May 31, 2019

Hi @samselikoff sorry for the newb question but how can i see/resolve the travis lint failure?

I ran ember-try locally and everything seemed ok.

As for upgrading I think, in theory, everything should work without any changes because people already using SASS in their apps will be able to import as they already are doing - and people using CSS will now be able to import the CSS files directly.

I cant really envisage a situation where someone will need to alter their files after the update.

Im on holiday right now but will try to get some internet access and have a go fixing this so it can be merged if theres something i need to do. Thanks!

@samselikoff
Copy link
Collaborator

No prob! First click that "Details" link down there next to the build failure, then you'll end up here. If you scroll a bit you'll see

image

which shows the yarn lint:js command failed

@samselikoff
Copy link
Collaborator

I guess only upgrade notes would be to tell people they can remove ember-cli-sass if this is the only reason they were installing it, which is unlikely... but we could put a note about it somewhere

@chrism
Copy link
Contributor Author

chrism commented May 31, 2019

Thanks @samselikoff.

I've fixed the lint issue and added some upgrading text to the readme now.

Hopefully Travis should pass now.

@samselikoff samselikoff self-requested a review May 31, 2019 21:01
@samselikoff
Copy link
Collaborator

LGMT! @lukemelia ?

@samselikoff
Copy link
Collaborator

think its a pretty simple change

Copy link
Contributor

@lukemelia lukemelia left a comment

Choose a reason for hiding this comment

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

👍

@lukemelia
Copy link
Contributor

Thanks for your work on this @chrism.

Feel free to merge @samselikoff. Otherwise, I'll try to get to it over the weekend.

@samselikoff samselikoff merged commit cf08c86 into yapplabs:master May 31, 2019
@chrism
Copy link
Contributor Author

chrism commented Jun 1, 2019

😁

@Turbo87
Copy link
Contributor

Turbo87 commented Jul 24, 2019

just FYI, the README is now somewhat misleading for SASS users due to https://stackoverflow.com/questions/7111610/import-regular-css-file-in-scss-file/36166487#36166487

tl;dr if you @import the CSS files in SASS you should drop the .css file extension, otherwise the files won't be imported correctly.

@chrism
Copy link
Contributor Author

chrism commented Jul 24, 2019

I’d hoped that is pretty clearly explained in the documentation here...

https://github.com/yapplabs/ember-modal-dialog/blob/master/README.md#configuring-styles

Where I even included both examples with and without the suffix?

@Turbo87
Copy link
Contributor

Turbo87 commented Jul 24, 2019

wow, I must have been blind 😂

sorry about the noise! 🤐

@chrism
Copy link
Contributor Author

chrism commented Jul 24, 2019

Its all good 😁👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants