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

feat(webpack): Render for each locale #26

Merged
merged 1 commit into from
Jul 17, 2017
Merged

feat(webpack): Render for each locale #26

merged 1 commit into from
Jul 17, 2017

Conversation

mattcompiles
Copy link
Contributor

Will add docs. Just opening for discussion.

@markdalgleish markdalgleish changed the title feat(webpack): render for each locale feat(webpack): Render for each locale Jul 14, 2017
@markdalgleish
Copy link
Member

I love how little code this takes 😍

Copy link
Contributor

@mengtzu mengtzu left a comment

Choose a reason for hiding this comment

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

:shipitparrot:

@markdalgleish
Copy link
Member

@MattsJones looks like this hasn't gone through Prettier.

@mattcompiles mattcompiles force-pushed the locales branch 2 times, most recently from 43193dc to 388167d Compare July 17, 2017 03:54
@@ -222,7 +222,17 @@ const buildWebpackConfigs = builds.map(({ name, paths, env }) => {
},
plugins: [
new webpack.DefinePlugin(envVars),
new StaticSiteGeneratorPlugin()
...locales.map(
Copy link
Member

Choose a reason for hiding this comment

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

The documentation says it will only render a single file in dev mode, but this appears to always map over every locale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow I missed actually implementing this 🤦 Will do.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect example of why providing docs with a PR is great for reviewers 😄

README.md Outdated
@@ -248,6 +248,39 @@ module.exports = {
}
```

### Locales

Often we render multiple versions of our application for different locations. Eg. Australia & New Zealand. To render an html file for each location you can use the locales option in `sku.config.js`.
Copy link
Member

Choose a reason for hiding this comment

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

Couple of tweaks:

  • Should be "locations, e.g. Australia and New Zealand"
  • Should be "HTML"

README.md Outdated

Often we render multiple versions of our application for different locations. Eg. Australia & New Zealand. To render an html file for each location you can use the locales option in `sku.config.js`.

Locales accepts an array of strings representing each locale you want to render html files for.
Copy link
Member

Choose a reason for hiding this comment

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

When referencing an option/argument name, we should use locales instead of "Locales". Also, you'll need to re-order this sentence so it's not the first word, otherwise it'll look weird.

(Also need to update to "HTML" here too)

README.md Outdated
}
```

For each locale, sku will call your `render.js` function and pass it the locale as a local.
Copy link
Member

Choose a reason for hiding this comment

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

Feel like using "locale" and "local" so close together is confusing. In fact, in this case I think you could avoid using the word "local" entirely.

README.md Outdated
)
```

The name of the html file that is generated will be suffixed by `-{locale}`.
Copy link
Member

Choose a reason for hiding this comment

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

"HTML" here too.

@@ -222,7 +222,17 @@ const buildWebpackConfigs = builds.map(({ name, paths, env }) => {
},
plugins: [
new webpack.DefinePlugin(envVars),
new StaticSiteGeneratorPlugin()
...locales.slice(0, isProductionBuild ? locales.length : 1).map(
Copy link
Member

Choose a reason for hiding this comment

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

Ooooh, nice 👏

@markdalgleish
Copy link
Member

Looks good, just one minor suggestion—might be good to explain when and why this is preferable to monorepos, talking about how we generate a single set of assets that supports both locales.

README.md Outdated
@@ -248,6 +248,39 @@ module.exports = {
}
```

### Locales

Often we render multiple versions of our application for different locations, eg. Australia & New Zealand. To render an HTML file for each location you can use the locales option in `sku.config.js`. Locales are preferable to [Monorepos](monorepo-support) when you need to render multiple versions of your HTML file but only need one version of each of the assets (JS, CSS, images, etc). Note: You can use `locales` inside a monorepo project.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry—I think monorepo-support needs to be #monorepo-support for the link to work correctly. Also, "Monorepos" should be lowercase.

Copy link
Member

@markdalgleish markdalgleish left a comment

Choose a reason for hiding this comment

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

Nice one, love this 👍

@mattcompiles mattcompiles merged commit c4b2a98 into master Jul 17, 2017
@mattcompiles mattcompiles deleted the locales branch July 17, 2017 05:23
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.

None yet

4 participants