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

Integrate elm-css #137

Closed
wants to merge 1 commit into from
Closed

Conversation

MazeChaZer
Copy link
Contributor

Tracked in #134

I tried to implement this as we discussed.

The stylesheet module is called Stylesheets, not Stylesheet, because I coundn't find a way to make elm-css-webpack-loader load a different module. In all the elm-css examples I saw, the plural version of the name was used, so this might not be so bad.

I included the noParse workaround you proposed.

There is an example Stylesheets module which is included in index.js and elm-css dependencies are added to elm-package.json.

@MazeChaZer
Copy link
Contributor Author

It seems that the elm-css binary has to be installed on the user's system for this to work.

@@ -10,7 +10,9 @@
"exposed-modules": [],
"dependencies": {
"elm-lang/core": "5.0.0 <= v < 6.0.0",
"elm-lang/html": "2.0.0 <= v < 3.0.0"
"elm-lang/html": "2.0.0 <= v < 3.0.0",
"rtfeldman/elm-css": "8.2.0 <= v < 9.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

I'm afraid elm-css is supposed to be purely optional, the template should be kept in its initial state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem 👍

@@ -1,4 +1,5 @@
import './main.css'
import './Stylesheets.elm'
Copy link
Owner

Choose a reason for hiding this comment

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

This kind of stuff should be documented in User Guide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sounds reasonable. Should I also delete the file Stylesheets.elm and instead provide an example on how to create this file in the user guide?

Copy link
Owner

Choose a reason for hiding this comment

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

@MazeChaZer yes, that sound good 👍

@halfzebra
Copy link
Owner

Good job, the progress so far looks promising. Please ping me if you're having troubles with elm-css binary.

@MazeChaZer MazeChaZer force-pushed the feature/elm-css branch 2 times, most recently from 9f7a797 to 6fcfef1 Compare June 13, 2017 09:29
@MazeChaZer MazeChaZer changed the title WIP: Integrate elm-css Integrate elm-css Jun 13, 2017
@MazeChaZer
Copy link
Contributor Author

Update on the current status:

  • All elm-css related stuff is in the user guide now
  • When built, the output of the Elm Stylesheet is emitted into the main.css, together with the normal CSS file

From my side this pull request would be ready to be merged ⭐

@halfzebra
Copy link
Owner

@MazeChaZer great job!

Thank you for this PR 👍

This code will go into master when #138 is merged.

@MazeChaZer
Copy link
Contributor Author

@halfzebra Great, just ping me if you need a rebase :)

@MazeChaZer
Copy link
Contributor Author

Just rebased this PR on top of the current master. I included the %PUBLIC_URL% placeholder replacement for Elm stylesheets.

@halfzebra
Copy link
Owner

halfzebra commented Jun 29, 2017

@MazeChaZer Sorry, it took us so long to get back to this PR. #135 was taking way too much time.

We've been discussing elm-css and the idea of having the automated setup for it in Create Elm App at #elm-dev since Elm Europe 2017.

@rtfeldman pointed out that style-elements might become a successor of elm-css in the future and it does not require any additional setup(which is a win).

@eeue56 have recommended on having a designated chapter in User Guide instead of introducing some special handling of an elm-css stylesheet.

I feel like the community consensus at the moment is leaning towards the idea of not including any elm-css-specific configuration into Create Elm App.

I know you have been working hard on that and that you did a very good job. We'd love to keep the docs and have the guide about elm-css setup in the User Guide but without additional changes to the Webpack configuration.

I'm sorry we couldn't accept the PR in it's current state, but I'm also pretty sure that, the documentation you've made, can be adjusted to cover elm-css setup and be an extremely valuable addition to User Guide.

Copy link
Owner

@halfzebra halfzebra left a comment

Choose a reason for hiding this comment

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

Please remove elm-css specific webpack configuration

@RyanCCollins
Copy link

I am excited about this one. Nice work! 😄

@halfzebra
Copy link
Owner

halfzebra commented Jul 11, 2017

@RyanCCollins thanks for the feedback!

Initially, we intended to close this PR, because elm-css does not seem as a recommended way of doing CSS in Elm projects and having that little integration will complicate the configuration for people, who eject.

If there's more interest in having a more friendly elm-css integration, it's always possible to get it in.

Could you please elaborate, why are you interested in elm-css?

Have you tried style-elements?

@RyanCCollins
Copy link

RyanCCollins commented Jul 11, 2017

Hey @halfzebra, I just re-read your comment and considering that Richard is saying that elm-styled is the future, I'd be interested in it over elm-css. I like the idea of having some kind of typed css api either way.

@eeue56
Copy link

eeue56 commented Jul 11, 2017

elm-styled is not what Richard thinks is the way forward. elm-styled uses some hacky native code to do some pretty risky stuff. style-elements is the project you're looking for

@halfzebra
Copy link
Owner

halfzebra commented Jul 11, 2017

@RyanCCollins the main advantage of style-elements is that it does not require additional setup. Richard's opinion was that there is no recommended way of doing CSS in Elm.

@MazeChaZer I'm going to refine this PR and add sections to the User Guide covering the setup for elm-css and elm-styled.

@halfzebra
Copy link
Owner

halfzebra commented Jul 11, 2017

@eeue56 my bad, I have been referring to the wrong package 😦

Thanks for pointing that out!

@MazeChaZer
Copy link
Contributor Author

@halfzebra Thanks, sorry for not responding earlier. We probably won't be using Elm in production in the long run, so this issue wasn't as important to me as it was when I started this pull request. I understand your decision to keep elm-css out of the default webpack config.


```sh
elm-app package install rtfeldman/elm-css
elm-app package install elm-css-helpers

Choose a reason for hiding this comment

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

This command doesn't work, it says:

Uh oh, argument "elm-css-helpers" is not a valid PACKAGE

There should be a slash separating the user and project name (user/project)

So it should be:

elm-app package install rtfeldman/elm-css-helpers

@halfzebra halfzebra closed this Sep 17, 2017
This was referenced Sep 17, 2017
@MazeChaZer MazeChaZer deleted the feature/elm-css branch September 18, 2017 06:23
@MazeChaZer
Copy link
Contributor Author

@halfzebra Thanks for taking care of this :)

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.

6 participants