CustomSelectControl V2 legacy adapter: stabilize experimental props #63248
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
85 tasks
ciampo
changed the title
|
Size Change: +48 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/custom-select-control-v2-legacy-adapter-cleanup-experimental-props
branch
from
604400a
to
67b90ea
Compare
ciampo
requested review from
ajitbohra,
tellthemachines and
ellatrix
as code owners
ciampo
commented
Jul 8, 2024
| @@ -60,56 +85,56 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) { | |||
| defaultValue: options[ 0 ]?.name, | |||
| } ); | |||
|
|
|||
| const children = options.map( | |||
There was a problem hiding this comment.
The following block of code is just about changing from mostly __experimentalHint to hint, the rest is whitespace-related changes (indentation)
|
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. |
ciampo
commented
Jul 8, 2024
| > | ||
| { currentHint?.__experimentalHint } | ||
| </Styled.SelectedExperimentalHintItem> | ||
| { selectedOptionHint && ( |
There was a problem hiding this comment.
Previously, we were rendering this component even where there wasn't a hint to show
ciampo
commented
Jul 8, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in c0e9f4b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9842567621
|
mirka
reviewed
Jul 10, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
ciampo
force-pushed
the
feat/custom-select-control-v2-legacy-adapter-cleanup-experimental-props
branch
from
c0e9f4b
to
2c73947
Compare
ciampo
commented
Jul 10, 2024
Comment on lines
+28
to
+36
| function applyOptionDeprecations( { | ||
| __experimentalHint, | ||
| ...rest | ||
| }: LegacyCustomSelectProps[ 'options' ][ number ] ) { | ||
| return { | ||
| hint: __experimentalHint, | ||
| ...rest, | ||
| }; | ||
| } |
There was a problem hiding this comment.
@mirka Relatively to #63248 (comment), I applied your suggestion of adapting the options array at runtime where necessary
tyxla
reviewed
Jul 10, 2024
packages/components/src/custom-select-control-v2/legacy-component/index.tsx
Outdated
Show resolved
Hide resolved
| > | ||
| { currentHint?.__experimentalHint } | ||
| </Styled.SelectedExperimentalHintItem> | ||
| { selectedOptionHint && ( |
That should be ok to leave as-is for now — the Storybook examples will be updated when we remove the downshift-based implementation (see proposed change). It shouldn't otherwise impact the component negatively, since it should be support the older version of the prop for backwards compat. |
…mentalShowSelectedHint
This avoids breakages for any consumer relying on object identity
ciampo
force-pushed
the
feat/custom-select-control-v2-legacy-adapter-cleanup-experimental-props
branch
from
2c73947
to
4d118af
Compare
But these stories test the v2, so given that we already deprecated the experimental prop, I'd expect we override the options to use the stable prop. Not a deal breaker, but can be confusing right now IMHO. |
tyxla
approved these changes
Jul 10, 2024
ciampo
deleted the
feat/custom-select-control-v2-legacy-adapter-cleanup-experimental-props
branch
ciampo
added
the
Needs Dev Note
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…ordPress#63248) * CustomSelectControl V2: add showSelectedHint prop, deprecate __experimentalShowSelectedHint * Update showSelectedHint unit tests * CustomSelectControl V2: add hint option prop, deprecate __experimentalHint * Rename __experimentalHint to hint * Refactor to smarter approach * Continue using `__experimentalHint` in the V1 storybook example * CHANGELOG * Add hint to options only if necessary * __experimentalShowSelectedHint => showSelectedHint * Move changelog entry to unreleased section * Give priority to `showSelectedHint` over `__experimentalShowSelectedHint` * Move `__experimentalHint` => `hint` code to run at runtime This avoids breakages for any consumer relying on object identity * Fix wrong link in inline comment --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
97 tasks
ciampo
added
has dev note
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 #55023
Stabilize the
__experimentalShowSelectedHintandoptions[]. __experimentalHintprops ofCustomSelectControlV2 legacy adapter by providing an equivalent, un-prefixed version:showSelectedHintandoptions[].hintWhy?
As we're about to swap the current V1 implementation with a new implementation based on
ariakit(ie. the V2 legacy adapter), it's also a good time to stabilize those props in preparation for the swap.Once the swap happens, we will also be able to remove the private
CustomSelectControlexport.How?
By soft-deprecating those props. Existing consumers using those props won't experience any breakages or warnings, but the deprecated props will show as such in the docs.
Testing Instructions
showSelectedHintprop.__experimentalShowSelectedHintis not shows anymore✍️ Dev note
See dev note in #63258