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: support to change the lang attribute in a preview HTML #15541

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Jul 9, 2021

Issue: #11706

What I did

I've added the STORYBOOK_PREVIEW_HTML_LANG environment variable to update the lang attribute in the preview html because I'd like to use storybook for non-English applications.

I've used an environment variable for this because the lang attribute is defined in a template for html-webpack-plugin and I didn't come up with a better way.

How to test

Should I add the documentation for STORYBOOK_PREVIEW_HTML_LANG to merge this PR?

@nx-cloud
Copy link

nx-cloud bot commented Jul 9, 2021

Nx Cloud Report

CI ran the following commands for commit 713ad3a. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@stale
Copy link

stale bot commented Jan 9, 2022

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 9, 2022
@ndelangen
Copy link
Member

@koba04 I like this feature, but I think setting this via an environment variable isn't a scalable way to configure a lot of things... That's what we have main.js for an addons/presets.

In addition I can see the changes you made are in fact quite solid and simple, however, because of work we did on our side, merging this has become really hard. A lot of code moved around, got renamed, and refactored, to support more things (webpack 4 & 5 (and now also vite)), support multiple versions of other packages, and improve performance, to name a few things.

So here's what I propose we do:
If you're still interested in working on this, we should start fresh. Made the changes based on either next or future/base (those 2 branches will meet up soon).

And then we'll want to take the value of the language from a preset.apply call. you can find an example of that here:

const headHtmlSnippet = await presets.apply('previewHead');

It should not be far more complex than what you've currently done.

I hope you're still interested in contributing. I realize we've left this PR completely discarded for way way too long, and I understand how that might have given you the impression that we didn't care.

We do care, your PR is very appreciated. Thank you for your hard work, effort and time.

I can close this PR if you'd indeed like to start fresh. resolving these merge conflict isn't really useful use of your time IMHO.

@stale stale bot removed the inactive label Jun 30, 2022
@koba04
Copy link
Contributor Author

koba04 commented Jul 5, 2022

@ndelangen Thank you!

And then we'll want to take the value of the language from a preset.apply call. you can find an example of that here:

That's definitely a better way than using environment variables!
I'll work on this and create a PR again!

@ndelangen ndelangen closed this Sep 13, 2022
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