BorderBoxControl: Promote to stable #65586
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DaniGuardiola
added
[Type] Code Quality
DaniGuardiola
requested review from
a team,
aristath and
noisysocks
and removed request for
a team
14 tasks
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +3 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting together these stabilization PRs @DaniGuardiola 👍
I've given this a test and it is pretty close. Within the block and site editors the component works as expected. I did run into a minor issue with the Stylebook examples though.
✅ Component unit tests pass
✅ Storybook redirect works as advertised
❓ There appears to be a typo in the storybook title
❓ The component's styles appear messed up however fixing the typo fixes them up.
✅ Didn't spot any regressions in the post/site editors' uses of the component
❓ Left a question around the docs changes but it doesn't have to be a blocker
✅ Consistent with BorderControl stabilization approach settle on in #65475
| LInked | Unlinked |
|---|---|
 |
 |
packages/components/src/border-box-control/stories/index.story.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/border-box-control/border-box-control/component.tsx
Show resolved
Hide resolved
…tabilize/border-box-control
tyxla
reviewed
Sep 24, 2024
There was a problem hiding this comment.
This looks good and tests well 👍
I'm a bit torn about the __experimentalIsRenderedInSidebar prop. As an external consumer, I find it consuming. Even if this prop exists, I think it should have a different name that reflects the behavior of the underlying component. IIRC it was specifying some alternative popover props under the hood. Why does that have to be sidebar-specific behavior? Feels like the sidebar shouldn't have to do anything to do with BorderBoxControl at all.
I'm also concerned that the component continues to export experimental util APIs, which we're not stabilizing here. It feels like we're doing half the work and not really assessing the stabilization of all the APIs of this component.
There's also a minor Storybook thing, I've left a separate comment about it.
Comment on lines
39
to
41
| hasSplitBorders as __experimentalHasSplitBorders, | ||
| isDefinedBorder as __experimentalIsDefinedBorder, | ||
| isEmptyBorder as __experimentalIsEmptyBorder, |
There was a problem hiding this comment.
What about those experimental utils? They are part of the component's API, but they remain experimental. Should we stabilize them as well?
There was a problem hiding this comment.
This is something I want to address in a follow-up. I already gave it a few iterations some time ago, and even paired on it with Marcelo over a donut chat which resulted in a draft PR I plan on reviving. There was some consensus about not exporting these from the component package, though the details remain to be discussed and iterated on.
I don't think it should block stabilization of this component though.
There was a problem hiding this comment.
I don't think it should block stabilization of this component though.
Why, though, aren't those part of the component's public API? I'd expect it would be the other way around, and we'd figure these out first, especially if we plan not to export them from the components package / deprecate them.
There was a problem hiding this comment.
Related: #61135
There was a problem hiding this comment.
Why, though, aren't those part of the component's public API? I'd expect it would be the other way around, and we'd figure these out first, especially if we plan not to export them from the components package / deprecate them.
I'm not strongly opposed that that, but to answer your question, my opinion is: no, they are not part of the component's API. I think these utils shouldn't have been exported here in the first place, and it's just an odd occurrence of misplaced utils. They're definitely related to the component, in that they apply to borders, but the component is its own thing.
So, the way I see it: stabilizing the component, moving the utilities somewhere else - these are separate tasks. Should they be tackled somewhat close to each other? I think so. Should one block the other? I think not.
That's just my 2 cents though. I'm good to go with another approach if someone feels more strongly about it.
There was a problem hiding this comment.
Fair enough. Let's keep the utils a separate problem and solve it as part of #61135 👍
8 tasks
|
We need to decide how we want to proceed regarding Here, @tyxla seems to be arguing against it. That's fine, I probably even agree with it though I generally do not have any strong feelings about either approach. However, we should make a decision once and apply it consistently across components, especially since this prop is shared by a few of them. What do you think? cc @WordPress/gutenberg-components |
Definitely don't intend to block the PR with this - I also acknowledge the prop is used across multiple components and it might make more sense to be addressed separately. Was just sharing my $0.02, but don't consider this a blocking comment. Maybe if we take care of #65581 not long after stabilizing the component, it should be OK. |
…tabilize/border-box-control
This was referenced Oct 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Part of #59418
Promote
BorderBoxControlto a non-experimental component.How?
__experimentalprefix.__experimentalexport for backwards compatibility.__experimentalin GB and components to the one without the prefix (including in Storybook stories).id(get it from the storybook URL) to thePREVIOUSLY_EXPERIMENTAL_COMPONENTSconst array inmanager-head.htmlso that old experimental story paths are redirected to the new one.Docs tweaked slightly. No unstable props (other than the 40px sizes one, and
__experimentalIsRenderedInSidebarwhich is tracked in #65581). If no one has any concerns, this one should be a straightforward merge.