CustomSelectControlV2: tweak item inline padding based on size #62850
Conversation
ciampo
force-pushed
the
fix/custom-select-control-v2-item-size
branch
from
44855d9 to
8012f91
Compare
ciampo
changed the title
|
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
added
[Type] Bug
ciampo
force-pushed
the
fix/custom-select-control-v2-item-size
branch
from
829974f to
03cff84
Compare
|
|
||
| export const SelectedItemCheck = styled( Ariakit.SelectItemCheck )` | ||
| display: flex; | ||
| align-items: center; | ||
| margin-inline-start: ${ ITEM_PADDING }; | ||
| margin-inline-start: ${ space( 2 ) }; |
There was a problem hiding this comment.
It's not immediately clear why we're using space( 2 ) here but we use the hardcoded INLINE_PADDING values in other areas.
There was a problem hiding this comment.
I stopped using the ITEM_PADDING variable because, IMO, the space between the item's contents and the checkmark is not tied to the item padding. Using that variable was misleading (especially as, in this PR, we add variable padding based on the size prop). It just so happened that the margin before the check icon is the same as the padding for the small/compact variant of the component, but that's a coincidence
| @@ -13,7 +13,11 @@ import { space } from '../utils/space'; | |||
| import { chevronIconSize } from '../select-control/styles/select-control-styles'; | |||
| import type { CustomSelectButtonSize } from './types'; | |||
|
|
|||
| const ITEM_PADDING = space( 2 ); | |||
| const INLINE_PADDING = { | |||
| compact: 8, // space(2) | |||
There was a problem hiding this comment.
I wonder if a better way would be to just export GRID_BASE and do a compact: GRID_BASE * 2 calculation here?
There was a problem hiding this comment.
I see the reason behind your point, at the same time it feels this would warrant a bigger, separate conversation about the grid system and related utilities. I'd be inclined to keep it as-is and refactor (if necessary) separately.
ciampo
force-pushed
the
fix/custom-select-control-v2-item-size
branch
from
03cff84 to
c49a4f3
Compare
…ress#62850) * Use context to forward size down to the option item * Apply different paddings based on item size, matching the select button * Use shared variables * CHANGELOG * Extract size calculations outside of styled element function --- Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org>
What?
Part of #55023
Match the
CustomSelectControlV2 item inline padding with the dropdown trigger (button)Why?
For better visual polish, and to match the V1 version of the component
How?
sizeprop from the root component to the item component via internal contextsizeto apply the same horizontal padding that is applied to the select trigger buttonIn the process, I also refactored some common inline padding values to a separate variable
Testing Instructions
CustomSelectControlV2Storybook examplesizeprop (and, optionally, the__next40pxDefaultSizeprop)Screenshots or screencast
size.alignment.-.cscv1.mp4
size.alignment.-.cscv2.before.mp4
size.alignment.-.cscv2.after.mp4