Components: replace TabPanel with Tabs in the editor's ColorGradientControl
#56351
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
|
Size Change: +22 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Nov 21, 2023
| } | ||
| > | ||
| <Tabs.TabList> | ||
| <Tabs.Tab id={ 'color' }> |
There was a problem hiding this comment.
When passing a string prop, we should just pass it as a string, that way it won't be necessary to tell React to further evaluate it as an expression. That suggestion applies to the rest of the strings that are passed as expressions in this PR.
Suggested change
| <Tabs.Tab id={ 'color' }> | |
| <Tabs.Tab id="color"> |
Furthermore, is it possible that the ID we're passing here may already be in use on the same page because it's a rather common one? Should we prefix it with something?
There was a problem hiding this comment.
Good points, I should have added a comment to that bit.
Passing a string here will trigger ESLint's error asking for useInstanceID, which I don't actually want to do. I should probably add an ignore comment with an explanation rather than going around it in this admittedly opaque way.
Tabs applies useInstanceId internally, so there id clashes shouldn't happen but, this comment has me realizing I should really take another look at that part of the component's API, as it really isn't going to be clear to consumers that will run into this exact situation. Putting this on my todo/followup list for Tabs.
30 tasks
ciampo
reviewed
Nov 23, 2023
ciampo
reviewed
Nov 29, 2023
packages/block-editor/src/components/colors-gradients/control.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/colors-gradients/control.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/colors-gradients/control.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
chad1008
force-pushed
the
tabs-color-gradient-control
branch
from
2f16b4a
to
4b48fe1
Compare
|
Flaky tests detected in e0a7f53. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7106110913
|
chad1008
force-pushed
the
tabs-color-gradient-control
branch
from
501d31e
to
e0a7f53
Compare
|
I've addressed the final CI failures, which were mobile/native tests. The problem was that calling the How I Got Here
Question |
dcalhoun
added
the
Mobile App - Automation
Thanks for the ping and including such a great overview in your comment! 🙇🏻 From what I gather from the situation, the workaround seems appropriate. I believe the mobile team has a couple of PRs open working to address the circular dependencies, so hopefully we can remove the workaround in the near future. It does not appear that the mobile editor relies upon For good measure, I added the "Mobile App - Automation" label to this PR so that the larger E2E test suite runs in wordpress-mobile/gutenberg-mobile#6447. If the CI checks pass on the sibling PR, I believe we should merge this PR. 🚀 |
|
Excellent, thank you so much @dcalhoun! I'll monitor those CI checks, and once they have passed I'll get this PR merged. |
|
The CI checks in wordpress-mobile/gutenberg-mobile#6447 passed. We are good to merge. 🚀 |
dcalhoun
removed
the
Mobile App - Automation
2 tasks
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?
Replaces the legacy
TabPanelcomponent with the newTabscomponent.Why?
Part of the work outlined in #52997
How?
TabPanelis swapped out forTabsand its sub-components. This eliminated the needs for aTABS_SETTINGSarray. I did opt to keep thetabPanelsarray to make it cleaner to address the two different cases where those panels are rendered (one as aTabPabelinsideTabs, an one as a standalonedivwhen only one option is being rendered.So far unit tests don't seem negatively impacted by these changes, but I'm going to wait for the full CI suite to run. Similarly I didn't see anything in the related styles that needed to be removed as a result of this migration.
Testing Instructions
Stylestab.ColorGradientControllooks and behaves the same way it does ontrunkColorGradientControland then open it again, it selects the right tab (ie, if the block has a gradient background, it should automatically open to the Gradient tab)Testing Instructions for Keyboard
ColorGradientControlopens.SolidandGradienttabstabpanel. It should focus the custom color button or gradient control points)