-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Add more prominent documentation around popover use #39709
Conversation
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.
This reads well to me @aaronrobertshaw, thanks for following up with an update to the docs!
For the URLs linking the documents to each other, I was wondering if they need to be absolute paths instead so that the URLs get transformed correctly when the docs are rolled out to the block editor handbook (e.g. https://developer.wordpress.org/block-editor/reference-guides/components/)
I'm not too sure how it's all hooked together, but I see in the Button component for example that a link to the icon readme is written as [Icon](/packages/components/src/icon/README.md)
.
Good catch @andrewserong.
This was an oversight on my behalf. I meant to update them as I copied them in. |
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 the update, LGTM! 🎉
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, thanks for improving the docs! The explanation is very helpful.
If this only affects consumers that are building for outside the editor, should we perhaps lead with that information? (e.g. "If you're using the component outside of the editor, make sure to...") I'm guessing the majority of wp-component consumers are building for inside the editor, so they don't need to add their own SlotFill.
packages/components/src/border-box-control/border-box-control/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/border-control/border-control/README.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Lena Morita <lena@jaguchi.com>
7a96330
to
5e56284
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.
LGTM with the suggested changes included -
What?
Highlights usage of the
Popover
andTooltip
components to ensure correct positioning.Why?
When using the
Tooltip
component, and thereforePopover
by proxy, outside the block or site editors, it isn't immediately obvious why eachPopover
is being rendered inline and out of position.This was encountered during the development of new border controls where improving the docs was suggested.
How?
A prominent section relating to using
Popover
andToolTip
components is added to the main readme for the components package which can then be linked to from individual component docs that are likely to needPopover
s rendered via aSlotFill
.Testing Instructions
Proofread edited docs.
ColorPalette
READMEBorderControl
READMEBorderBoxControl
README