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

Add Storybook template for i18n #4764

Merged
merged 56 commits into from Mar 30, 2022
Merged

Conversation

simoncrypta
Copy link
Collaborator

@simoncrypta simoncrypta commented Mar 15, 2022

This will add the possibility to read the translation string and also change the language in Storybook !

Need @storybook/addon-essentials which I think should be included by default in RedwoodJS. I will open another PR to discuss this really soon #4765.

I copy the task configure-storybook from ui folder to i18n, I don't know if we should make this a lib accessible for all setup.

Screenshot from 2022-03-15 16-58-33

@simoncrypta
Copy link
Collaborator Author

@Tobbe You can probably be interesting at this one

@Tobbe
Copy link
Member

Tobbe commented Mar 18, 2022

How can we make this compatible with also adding Chakra support? Support for merging SB configs instead of overwriting.

@thedavidprice
Copy link
Contributor

Thanks for moving this forward @simoncrypta and @Tobbe 🚀

@virtuoushub and @shilman Could I get your high-level review about this PR as well as #3515

Recently, Yann gave Pete and I some guidance that we should move away from currenlty having a StorybookProvider decorator and instead looking into loaders. I'm not sure if that direction has any overlap with how Simon is approaching i18n support here. So just making sure to close any potential communication gaps most of all!

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@netlify
Copy link

netlify bot commented Mar 20, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit e1b353a
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/6243fd8ae8b5e40008533124

simoncrypta and others added 2 commits March 20, 2022 14:05
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@simoncrypta
Copy link
Collaborator Author

simoncrypta commented Mar 20, 2022

@Tobbe I will look a way to not overwrite if the file is already create !
@thedavidprice I was looking loaders and I don't think this can be use for i18n plugin, I will try to dig more about this.

@simoncrypta
Copy link
Collaborator Author

How can we make this compatible with also adding Chakra support? Support for merging SB configs instead of overwriting.

I duplicate the configure-storybook file of Chakra to make it work with i18n, but I will prefer to have only one. Since we need to merge SB config with whatever that come first, using the same configure-storybook script make sense, but where did I put this file ? If it at /setup/tasks/configure-storybook.js do I need to write something to exclude it for yargs ?

@Tobbe
Copy link
Member

Tobbe commented Mar 24, 2022

but where did I put this file ? If it at /setup/tasks/configure-storybook.js do I need to write something to exclude it for yargs ?

I don't know. I think you just have to try it and see what happens :)

@simoncrypta
Copy link
Collaborator Author

Let's Go !

@thedavidprice
Copy link
Contributor

Nice work, everyone!

I really want to make sure @virtuoushub reviews and approves the Storybook config additions here. Looping him in.

simoncrypta and others added 7 commits March 29, 2022 13:51
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@simoncrypta
Copy link
Collaborator Author

@Tobbe Thanks again for your great reviews!
For #4953 Nothing to do for this
But for #4900, depend on who merge first, we just need to consider the new configureStorybook lib.


const insideNewStorybookConfigWithoutReactAndDecoration =
newStorybookPreview
.replace(/import *. as React from 'react'/, '')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.replace(/import *. as React from 'react'/, '')
.replace(/import *. as React from 'react'/, '')

Sorry I didn't see this earlier. Did you mean to make that .* (instead of *.)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I test the regex, I think is right, but I will double-check

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it matches "space" zero or more times, then "any character", then "space", etc. So it works. I just wasn't sure if that's what you were going for :)

Copy link
Member

Choose a reason for hiding this comment

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

You probably want /import \* as React from 'react'/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh! yeah right, I had tried this and didn't work, but I'm not the best at regex, so I will test again.

Copy link
Member

Choose a reason for hiding this comment

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

@simoncrypta
Copy link
Collaborator Author

simoncrypta commented Mar 29, 2022

@Tobbe, I wonder if this new lib overlaps on the rewrite of configureStorybook ?
setup-component-library.js
TBH, I like how it solved the problem, so no hard feeling if we use the other one as base. Just need to merge it first.

@Tobbe
Copy link
Member

Tobbe commented Mar 29, 2022

TBH, I like how it solved the problem, so no hard feeling if we use the other one as base. Just the to merge it first.

@simoncrypta I agree, it's nicely done. But I still think, if you just address my last couple of comments we can get this one in first, and then he can handle the merge conflicts and overwrite your changes with what he's got going on there.

Tobbe and others added 2 commits March 30, 2022 06:16
Co-authored-by: Peter Colapietro <petercolapietro+github@gmail.com>
@Tobbe Tobbe enabled auto-merge (squash) March 30, 2022 06:53
@Tobbe Tobbe merged commit dd9656b into redwoodjs:main Mar 30, 2022
@jtoar jtoar added this to the next-release milestone Mar 30, 2022
@Tobbe Tobbe mentioned this pull request Mar 30, 2022
4 tasks
dac09 added a commit to dac09/redwood that referenced this pull request Mar 31, 2022
…nto fix/subdirectory-routing

* 'fix/subdirectory-routing' of github.com:dac09/redwood:
  Clarify the cell aliasing section of tutorial (redwoodjs#4964)
  s/posts/articles (redwoodjs#4971)
  fix(deps): update dependency cross-undici-fetch to v0.1.28 (redwoodjs#4966)
  Add Storybook template for i18n (redwoodjs#4764)
  Add @storybook/addon-essentials by default (redwoodjs#4765)
@thedavidprice thedavidprice modified the milestones: next-release, v1.1.0 Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature topic/cli topic/storybook
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

None yet

5 participants