-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix Storybook preview config being set up in old config directory #11869
base: main
Are you sure you want to change the base?
Fix Storybook preview config being set up in old config directory #11869
Conversation
Also fixing (well, working around would be more accurate) this issue which would generate an invalid preview file for chakra-ui all along when running The root cause is that this redwood/packages/cli/src/lib/configureStorybook.js Lines 52 to 58 in a99bde0
correctly merges the file, however it doesn't filter out if the file it merges with already has a default export. Hence we need to ensure the i18n, mantine & chakra-ui setup templates do not contain a default export (which the first two don't) to work around this issue for the moment. |
See redwoodjs#11869 (comment) for explanation.
Thanks @Philzen 🙏 |
Done. With a few years of background in unit testing, looking at the concerned
|
This reverts commit f8a19c4.
See redwoodjs#11869 (comment) for explanation.
0edd4da
to
bcad634
Compare
…at least within this test. Ideally this would be exported from a central place.
bcad634
to
8b7a4d6
Compare
OK i may have gone a little crazy here, but i thought i'd give it a try to increase the test maintainability and hence reduce a little technical debt. Check cbacc5f...8b7a4d6 I could only save 480 lines of test code … which is better than nothing, but i probably promised a bit too much. As the implementation / single-source-of-truth is not exported in a consumable manner, i could only refactor the tests so they still repeat it … but now it at least only repeats it once except of four times and is much more readable IMHO. Thinking of #11760 where now we'd be able to delete four more lines out of this test and make everything more complete. Technically this is way out-of-scope – so @Tobbe if you want i can pick them over into a separate PR for cleaner separation of concerns. |
setup ui
&setup i18n
commands still use old storybook folders #11864i18n
dependency from i18n setup #11789preview.[tsx|js]
would contain two exports (see comment below for details)@Tobbe this should definitely go into the next patch release as it is broken since v8.0.0.
Got a couple of more optimizations for chakra-ui setup in the pipeline that can build on this but are non-critical additions that can go into a minor or major version bump.