-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Blocks: New Canvas API #20723
Blocks: New Canvas API #20723
Conversation
…torybookjs/storybook into jeppe/sb-1145-update-api-of-canvas-block
…-1145-update-api-of-canvas-block
…-1145-update-api-of-canvas-block
…-1145-update-api-of-canvas-block
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.
Looks great! Couple of details to nail down, and I think maybe we crossed wires over the layout prop.
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.
x
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.
LGTM, couple small final things
Children.forEach(children, (child) => { | ||
if (layout) { | ||
return; | ||
} | ||
layout = (child as ReactElement)?.props?.parameters?.layout; | ||
}); |
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.
Children.forEach(children, (child) => { | |
if (layout) { | |
return; | |
} | |
layout = (child as ReactElement)?.props?.parameters?.layout; | |
}); | |
const layout ??= | |
Children.map(children, | |
(child) => (child as ReactElement)?.props?.parameters?.layout | |
).find(Boolean); |
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.
I'm sorry I have to disagree here.
cleverness++, readability--
Fixes #12135
What I did
This PR changes the Canvas block to use the new
of
based API, while still being backwards compatible with the existing API, that is now deprecated and will be removed in 8.0.It also changes the
DocsStory
block to use the new API, without being backwards compatible as it is an undocumented component.All that's missing here is the migration docs, but they will be easier to write once #20699 lands.
How to test
See stories in published UI SB:
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]