CustomSelectControl V2: prevent keyboard event propagation in legacy wrapper #62907
Conversation
ciampo
force-pushed
the
fix/custom-select-control-v2-popover-keyboard-propagation
branch
from
922ef7f to
e2c7d63
Compare
ciampo
added
[Type] Bug
|
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
force-pushed
the
fix/custom-select-control-v2-popover-keyboard-propagation
branch
from
e2c7d63 to
6ecbde2
Compare
| @@ -70,9 +79,7 @@ export type _CustomSelectProps = CustomSelectButtonProps & { | |||
| label: string; | |||
| }; | |||
|
|
|||
| export type CustomSelectProps = _CustomSelectProps & | |||
| CustomSelectButtonProps & | |||
There was a problem hiding this comment.
Removed CustomSelectButtonProps since it's already part of _CustomSelectProps
There was a problem hiding this comment.
@mirka — just making sure that you see this change, since you refactored these types last
| export type _CustomSelectInternalProps = { | ||
| /** | ||
| * True if the consumer is emulating the legacy component behavior and look | ||
| */ | ||
| isLegacy?: boolean; | ||
| }; |
There was a problem hiding this comment.
I needed a way to allow the internal implementation to expose a isLegacy flag, without forwarding it to the public API of the V2 component. Not sure if we think there's a better way to do that? @mirka
There was a problem hiding this comment.
Can't think of a better way, TBH.
What a bummer that we have to do it with hacks like this. It's an indication that the legacy adapter approach has probably too many downsides and won't be a recommended one in the future.
There was a problem hiding this comment.
The legacy adapted approach definitely has its pros and cons.
It may not be worth keeping a common underlying implementation for V1 and V2 if we then have to abstract it and complicate it with extra props and exceptions. Let's keep it as-is for now and look back at it once V2 legacy wrapper has shipped and we look at V2
| export type _CustomSelectInternalProps = { | ||
| /** | ||
| * True if the consumer is emulating the legacy component behavior and look | ||
| */ | ||
| isLegacy?: boolean; | ||
| }; |
There was a problem hiding this comment.
Can't think of a better way, TBH.
What a bummer that we have to do it with hacks like this. It's an indication that the legacy adapter approach has probably too many downsides and won't be a recommended one in the future.
ciampo
deleted the
fix/custom-select-control-v2-popover-keyboard-propagation
branch
…ess#62907) * Add `isLegacy` prop to internal implementation * Stop keyboard event propagation for legacy V2 * Enable skipped test for V2 legacy * Add comment * CHANGELOG * Fix types --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
What?
Part of #55023
Prevent keyboard events propagation from the select popover in
CustomSelectControlV2legacy wrapperWhy?
To align the behavior of the V2 legacy wrapper to the V1 version of the component. Here's more context to why the V1 stops event propagation.
Note
I personally think that this behaviour is non-standard and shouldn't be retained for the V2 (non-legacy) version of the component.
How?
keydownevent propagation on the select dropdownTip
The "legacy" flag introduced in this PR can be further explored to trigger more differences in behaviour and styles as we further define specs of the V2 (non-legacy) version.
Testing Instructions
trunkversion of Storybook for the V2 legacy wrapper, and notice how the sidebar toggles between opened and closed