Skip to content

[Encore] Don't enable the Sass loader at the beginning of the First Example page #9526

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

Merged
merged 1 commit into from
Apr 16, 2018

Conversation

Lyrkan
Copy link
Contributor

@Lyrkan Lyrkan commented Mar 30, 2018

Someone pointed out in symfony/webpack-encore#287 that if you follow the First example page from top to bottom you are going to run yarn encore before being informed that calling enableSassLoader() implies adding additional dependencies (which is done just after).

This PR moves the related paragraph to warn the user that Encore will tell him about missing packages before he encounters the error.

I also added the "after running it" part to make it a bit more obvious that the information is going to be present in the command output.

Edit: Changed it so enableSassLoader() is commented before running Encore for the first time

@Lyrkan Lyrkan force-pushed the warn-missing-sass-dependencies branch from 24bd2de to c0cfd35 Compare March 30, 2018 15:55
@weaverryan
Copy link
Member

I also think the report is legit @Lyrkan. But, what about commenting out all of the features that require extra packages so that the first build is successful? Then we can show an example of uncommenting the enableSassLoader() so they can see what happens in that case (we could use the diff file type in the docs to show that one line being uncommented). I think that would be best.

@Lyrkan Lyrkan force-pushed the warn-missing-sass-dependencies branch from c0cfd35 to b071ed2 Compare March 31, 2018 18:53
@Lyrkan Lyrkan changed the title [Encore] Proactively warn the user that enableSassLoader() needs additional packages [Encore] Don't enable the Sass loader at the beginning of the First Example page Mar 31, 2018
@Lyrkan
Copy link
Contributor Author

Lyrkan commented Mar 31, 2018

@weaverryan I agree, should be better now :)

@javiereguiluz javiereguiluz changed the base branch from 3.3 to 3.4 April 16, 2018 13:10
@javiereguiluz javiereguiluz force-pushed the warn-missing-sass-dependencies branch from b071ed2 to 18586e2 Compare April 16, 2018 13:10
@javiereguiluz
Copy link
Member

@Lyrkan this was a great first contribution to Symfony Docs! Thanks a lot.

@javiereguiluz javiereguiluz merged commit 18586e2 into symfony:3.4 Apr 16, 2018
javiereguiluz added a commit that referenced this pull request Apr 16, 2018
… the First Example page (Lyrkan)

This PR was submitted for the 3.3 branch but it was merged into the 3.4 branch instead (closes #9526).

Discussion
----------

[Encore] Don't enable the Sass loader at the beginning of the First Example page

Someone pointed out in symfony/webpack-encore#287 that if you follow the [First example](http://symfony.com/doc/master/frontend/encore/simple-example.html) page from top to bottom you are going to run `yarn encore` before being informed that calling `enableSassLoader()` implies adding additional dependencies (which is done just after).

~~This PR moves the related paragraph to warn the user that Encore will tell him about missing packages before he encounters the error.~~

~~I also added the "after running it" part to make it a bit more obvious that the information is going to be present in the command output.~~

**Edit:** Changed it so `enableSassLoader()` is commented before running Encore for the first time

Commits
-------

18586e2 [Encore] Don't enable the Sass loader at the beginning of the First Example page
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