-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore: migrate to Storybook v6.0.x #17033
chore: migrate to Storybook v6.0.x #17033
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit a277f49:
|
Ok so those 2 deps are peerDeps of sass-loader (it's unfortunate that satisfied resolve this as an error) -> to fix it I'm gonna ignore sass completely within satisfied Hmm, CI fails on When I run |
It's not quite clear to me either from the satisfied docs (and I don't have background on why this check was added). My guess would be something about a missing or invalid peer dependency? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Some minor comments/questions on work so far.
74f5de4
to
3f4a6fa
Compare
this runs
debug troubleshooting so far:
Looks like that field is invalid, as webpack adds all supported extensions when trying to resolve this package:
after manual change to "browser": {
- "dist/react-textarea-autosize.cjs.js": "dist/react-textarea-autosize.cjs.browser.js",
+ "dist/react-textarea-autosize.cjs": "dist/react-textarea-autosize.cjs.browser.js",
- "dist/react-textarea-autosize.esm.js": "dist/react-textarea-autosize.esm.browser.js"
+ "dist/react-textarea-autosize.esm": "dist/react-textarea-autosize.esm.browser.js"
}, it is resolved properly.
|
8d6b305
to
25003fe
Compare
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Baseline commit: ada2cf5ba6f6adac058aa52fa5ff163e48f7576c (build) |
Issues update: 1. @fluentui/projects-test test is failing Note: this passes on my local machine
CI:UPDATE: after manual re-run it passed (CI is quite flaky) - https://uifabric.visualstudio.com/fabricpublic/_build/results?buildId=189317&view=logs&j=1e96f989-784f-546f-05bb-8f7d8dfe5399&t=1d2e21c4-a3ed-5b3f-9928-c286f8ffc322
any ideas @ecraig12345 @layershifter ? |
RE:
Seems like we should just review and approve these changes, right? |
I was not able to repro this issue locally on this branch. So yeah, it might be a flaky build, however I haven't seen any flakiness in this test before 🤔 This build is successful, can you please provide a link to a failed build? |
There is definitely a difference in padding on
|
.addDecorator(FabricDecoratorTall) |
I removed this call locally and got only single decorator afterwards.
I re-run the pipeline, probably got overridden by it. sorry about that |
It comes from Storybook: storybookjs/storybook#12109. I tried in export const parameters = { layout: 'fullscreen' } But then there is no |
yup -> storybookjs/storybook#12041 they added |
569549c
to
640bf5d
Compare
This should be now ready 🤞 |
@@ -41,3 +41,25 @@ export const FabricDecoratorFullWidth: DecoratorFunction<StoryFnReactReturnType> | |||
</div> | |||
</div> | |||
); | |||
|
|||
const mapper = { default: '', fixed: '300px', full: '100%' }; | |||
export function modifyDeprecatedDecoratorStyles(options: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this temporary? If yes, can you please add a small TODO/notice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. I added explicit comments per story, but good idea to add it here as well. thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job 👍 As @ecraig12345 mentioned I suggest to dedupe at least @babel/*
dependencies.
…id ES6 stories that have decorator duplication
bc31c48
to
a277f49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this!
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
* chore: migrate vr-tests and react-examples to SB v6 * chore: migrate fluentui/storybook to SB v6 because off syncpack policy * chore: ignore sass when running satisfied check * chore: remove source-map-loader from root to make satisfied happy * chore: remove json-loader which is not needed with webpack 5 * fix(fluentui/docs): fix invalid import which appeared after SB bump/TS single version policy * chore(vr-tests): turn on skip lib check as sb6 uses ecma modules normalization - esModuleInterop on * chore: move webpack to single version policy * chore: bump react-textarea-autosize which had invalid browser path for webpack * chore: migrate to html-wp-plugin 5 to make docs build work * chore(vr-tests): add remark about deprecated api * chore(vr-tests): add SB rendering canvas temporary workaround to remove screener regressions * chore(vr-tests): remove duplicate decorator insertion where possible * chore(vr-tests): introduce temporary workaround decorator for non valid ES6 stories that have decorator duplication * chore: apply yarn-dedupe on @babel scope
Pull request checklist
$ yarn change
Description of changes
require.resolve
would do the trick, but going forward single version policy is the way forward )/pacakges
until explicitly configured (syncpack restriction) so I bumped SB over there as wellFocus areas to test