Skip to content

docs(readme): add a note about media query extraction (media-query-plugin) #235

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Aug 21, 2018
Merged

docs(readme): add a note about media query extraction (media-query-plugin) #235

merged 4 commits into from
Aug 21, 2018

Conversation

SassNinja
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Breaking Changes

Additional Info

This PR updates the docs/readme of this repo and adds a section about using mini-css-extract-plugin together with my media-query-plugin.

Relates to #162

@evilebottnawi is this update ok for you?

@jsf-clabot
Copy link

jsf-clabot commented Aug 8, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky changed the title update docs with media-query-plugin section docs(readme): add extract media query example (media-query-plugin) Aug 8, 2018
@michael-ciniawsky michael-ciniawsky added this to the 0.5.0 milestone Aug 8, 2018
michael-ciniawsky
michael-ciniawsky requested changes Aug 8, 2018
View reviewed changes
README.md Outdated

Afterwards a mobile user only needs to load this

```scss
Copy link
Member

Choose a reason for hiding this comment

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

scss => css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this matter?
It's only about syntax highlighting and I first tried scss but then the @ of @media looks weird. Using css result in the best look here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never mind, I just noticed you were talking replacing the 2nd scss with css

done!

README.md Outdated
instead of this

```css
.foo { color: red }
Copy link
Member

Choose a reason for hiding this comment

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

'\n'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

README.md Outdated
.foo { color: red }
@media (min-width: 75em) {
.foo { color: blue }
}
Copy link
Member

Choose a reason for hiding this comment

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

'\n'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

README.md Outdated
To improve this you can use the [media-query-plugin](https://github.com/SassNinja/media-query-plugin) which plays together well with the mini-css-extract-plugin. It will extract the defined media queries and emit them as separate files (or inject into existing chunks if working with dynamic imports).

Afterwards a mobile user only needs to load this

Copy link
Member

Choose a reason for hiding this comment

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

Please add 'useful/expressive' filenames for context in the following format **`useful.css`** to all code examples, e.g `**webpack.config.js**`to the config examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@SassNinja
improve docs of media-query-plugin
2f363dd 
I've now added filenames and line breaks in CSS code for better readability.
@SassNinja SassNinja force-pushed the docs/media-query-plugin branch from 776de15 to 2f363dd Compare August 8, 2018 11:43
michael-ciniawsky
michael-ciniawsky approved these changes Aug 8, 2018
View reviewed changes
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@SassNinja Thx

sokra
sokra requested changes Aug 14, 2018
View reviewed changes
Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

This is too much text for promoting another plugin. The usage can be explained in the README of the plugin.

A single link with one sentence seem to be more appropriate for me.

@SassNinja
Copy link
Contributor Author

SassNinja commented Aug 14, 2018 •
edited
Loading

This is too much text for promoting another plugin. The usage can be explained in the README of the plugin.

A single link with one sentence seem to be more appropriate for me. – @sokra

I've added a more detailed description because @evilebottnawi asked me for an example.
So if I got you right you'd prefer something as the following?

### Other Webpack Plugins

This plugin plays together well with the following other webpack plugins.

#### media-query-plugin

If you'd like to extract the media queries from the extracted CSS (so mobile users don't need to load desktop specific CSS anymore) you should use the [media-query-plugin](https://github.com/SassNinja/media-query-plugin). It will take over the filename option of the mini-css-extract-plugin and recognize its generated CSS chunks.

or without considering other plugins

### Media Query Plugin

If you'd like to extract the media queries from the extracted CSS (so mobile users don't need to load desktop specific CSS anymore) you should use the [media-query-plugin](https://github.com/SassNinja/media-query-plugin). It will take over the filename option of the mini-css-extract-plugin and recognize its generated CSS chunks.

@michael-ciniawsky
Copy link
Member

I'm in favor of the latter :)

### Media Query Plugin

...

@SassNinja
Copy link
Contributor Author

alright, I've pushed the short version

michael-ciniawsky
michael-ciniawsky approved these changes Aug 15, 2018
View reviewed changes
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

@SassNinja Thx

README.md Outdated
```

For more information please check the media-query-plugin [docs](https://github.com/SassNinja/media-query-plugin) and [examples](https://github.com/SassNinja/media-query-plugin/tree/master/examples).
---
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not needed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed now

(hope the PR is now good to go)

@michael-ciniawsky michael-ciniawsky changed the title docs(readme): add extract media query example (media-query-plugin) docs(readme): add a note about media query extraction (media-query-plugin) Aug 15, 2018
@alexander-akait
Copy link
Member

@michael-ciniawsky Right now good, maybe in future (webpack@5) and with built-in support css we can implement this feature using splitChunks.cacheGroups

@michael-ciniawsky michael-ciniawsky merged commit a78b301 into webpack-contrib:master Aug 21, 2018
@michael-ciniawsky michael-ciniawsky removed this from the 0.5.0 milestone Aug 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

@michael-ciniawsky michael-ciniawsky michael-ciniawsky approved these changes

@sokra sokra Awaiting requested review from sokra

Assignees
No one assigned
Labels
semver: patch status: approved type: docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SassNinja @jsf-clabot @michael-ciniawsky @alexander-akait @sokra