CustomSelectControlV2: Rename for consistency #60178
Conversation
mirka
added
[Type] Code Quality
| @@ -26,4 +26,6 @@ export function CustomSelectItem( { | |||
| ); | |||
| } | |||
|
|
|||
| CustomSelectItem.displayName = 'CustomSelectControlV2.Item'; | |||
There was a problem hiding this comment.
This was necessary to ensure that React devtools and the Storybook code snippet generator displays the intended name.
There was a problem hiding this comment.
Perhaps this shouldn't be necessary if we exported them like this (untested):
const CustomSelectControlV2 = CustomSelect;
CustomSelectControlV2.Item = Item;
export default CustomSelectControlV2;
which would be something similar to what we're doing with Tabs right now.
There was a problem hiding this comment.
That doesn't seem to work, it looks like it just uses the name of the initial function no matter what.
And your comment made me realize that the Tabs subcomponents aren't showing the correct display names either 😅 I'll fix that.
|
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. |
| @@ -1,5 +1,9 @@ | |||
| /** | |||
| * Internal dependencies | |||
| */ | |||
| export { default as CustomSelect } from './default-component'; | |||
| export { default as CustomSelectItem } from './custom-select-item'; | |||
| import CustomSelect from './default-component'; | |||
There was a problem hiding this comment.
Should we also use CustomSelectControlV2 here for consistency?
| import CustomSelect from './default-component'; | ||
| import Item from './item'; | ||
|
|
||
| const CustomSelectControlV2 = Object.assign( CustomSelect, { Item } ); |
There was a problem hiding this comment.
Do we really need to use Object.assign() rather than just do a direct assignment?
There was a problem hiding this comment.
It is necessary for TypeScript reasons because here the component is already exported once and "sealed" to new keys.
But I moved the assignment to the default-component file so it's a bit simpler and doesn't require the Object.assign (7b678d4).
| @@ -26,4 +26,6 @@ export function CustomSelectItem( { | |||
| ); | |||
| } | |||
|
|
|||
| CustomSelectItem.displayName = 'CustomSelectControlV2.Item'; | |||
There was a problem hiding this comment.
Perhaps this shouldn't be necessary if we exported them like this (untested):
const CustomSelectControlV2 = CustomSelect;
CustomSelectControlV2.Item = Item;
export default CustomSelectControlV2;
which would be something similar to what we're doing with Tabs right now.
| @@ -12,7 +12,7 @@ import { useState } from '@wordpress/element'; | |||
| /** | |||
| * Internal dependencies | |||
| */ | |||
| import { CustomSelect as UncontrolledCustomSelect, CustomSelectItem } from '..'; | |||
| import UncontrolledCustomSelectControl from '..'; | |||
There was a problem hiding this comment.
Should we use UncontrolledCustomSelectControlV2 instead?
# Conflicts: # packages/components/src/custom-select-control-v2/stories/legacy.story.tsx
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
* CustomSelectControlV2: Rename for consistency * Add changelog * Improve naming consistency in tests * Simply subcomponent composition and exports * Apply suggestions to readme Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
* CustomSelectControlV2: Rename for consistency * Add changelog * Improve naming consistency in tests * Simply subcomponent composition and exports * Apply suggestions to readme Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
Part of #55023
What?
Renames
CustomSelectandCustomSelectItemto the names we actually want to ship with:CustomSelectControlV2CustomSelectControlV2.ItemWhy?
I have been discussing this with @ciampo and @brookewp, and we now have the guidelines documented in the Contributing guide.
We chose dot notation (
Component.Subcomponent) over flat namespacing (ComponentSubcomponent) for better ergonomics on the consumer side, when doing things like unlocking private APIs and importing components with multiple subcomponents.Testing Instructions
✅ Internal naming makes sense
✅ Documentation (readme and Storybook) is consistent