Skip to content

refactor: Apply padding to option-content-div instead of selectable-item outer-div #4070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SpyZzey merged 24 commits into from
Dec 4, 2025
Merged

refactor: Apply padding to option-content-div instead of selectable-item outer-div #4070

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
456ebf7
feat: selectable-item move padding to child option-content
SpyZzey Nov 21, 2025
72f67d4
fix: incorrect bottom padding
SpyZzey Nov 21, 2025
0ef250b
fix: padding
SpyZzey Nov 24, 2025
c92571d
fix: padding
SpyZzey Nov 24, 2025
6bbd824
feat: add no-content-padding
SpyZzey Nov 24, 2025
4d9ed48
refactor: remove stylelint-disable, move no-content-padding styling c…
SpyZzey Nov 25, 2025
edbdcf8
refactor: rename disableContentPadding -> disableContentPaddings
SpyZzey Nov 25, 2025
08c816f
refactor: remove default value for disableContentPaddings
SpyZzey Nov 25, 2025
a47c0d8
fix: padding
SpyZzey Nov 26, 2025
1a5e12d
feat: change disableContentPadding to disableContentStyling
SpyZzey Nov 26, 2025
fea65f8
fix: disabled color set when no-content-styling enabled
SpyZzey Nov 27, 2025
537bdcf
fix: padding too small on non-interactive elements
SpyZzey Nov 27, 2025
2c100cd
fix: padding on non-interactiveGroup parent
SpyZzey Nov 27, 2025
931e7dc
fix: remove disableContentStyling=true
SpyZzey Nov 27, 2025
aae6750
test: add disableContentStyling to selectable-item permutations
SpyZzey Nov 28, 2025
aa6681a
fix: disable state not correct color
SpyZzey Dec 1, 2025
d3fcd3d
refactor: add padding to selectable-item-content instead of option-co…
SpyZzey Dec 2, 2025
858ad32
fix: font-weight bold not on interactiveGroups
SpyZzey Dec 3, 2025
1c79026
fix: padding
SpyZzey Dec 3, 2025
5ec8b6b
test: move disable-content-styling prop to custom permutations page
SpyZzey Dec 4, 2025
0f14628
fix: disableContentStyling default value to false
SpyZzey Dec 4, 2025
bc825d9
fix: rename disable-content-styling.permutations.tsx to include .page
SpyZzey Dec 4, 2025
b87a2b4
refactor: remove unnecessary spacebetween, collapse css rules
SpyZzey Dec 4, 2025
fecea21
refactor: change title of dev-page selectable-item-disable-content-st…
SpyZzey Dec 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions pages/selectable-item/permutations.page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Option from '~components/internal/components/option';
import SelectableItem, { SelectableItemProps } from '~components/internal/components/selectable-item';
import SpaceBetween from '~components/space-between';

import createPermutations from '../utils/permutations';
import createPermutations, { ComponentPermutations } from '../utils/permutations';
import PermutationsView from '../utils/permutations-view';
import ScreenshotArea from '../utils/screenshot-area';

Expand All @@ -20,7 +20,7 @@ const optionHasBackground = <Option option={{ value: 'has background' }} />;
const optionGroupHeader = 'group header';
const childOption = 'child option';

const permutations = createPermutations<SelectableItemProps>([
let permutationsConfigs: ComponentPermutations<SelectableItemProps>[] = [
{
selected: [false],
highlighted: [false, true],
Expand Down Expand Up @@ -66,7 +66,15 @@ const permutations = createPermutations<SelectableItemProps>([
isChild: [true],
children: [childOption],
},
]);
];

// Copy permutations and add selectable-items with no-content-styling.
permutationsConfigs = [
...permutationsConfigs,
...permutationsConfigs.map(config => ({ ...config, disableContentStyling: [true] })),
Copy link
Member

@jperals jperals Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that adding these permutations will cause some operational friction as the visual regression tests will fail. My recommendation is to move the new permutation to a new page. And IMO they don't need to be covered by visual regression, because this is an internal option, not customer-facing.

Copy link
Member Author

@SpyZzey SpyZzey Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be customer facing once the arbitrary content for select/multiselect will go live

Copy link
Member

@jperals jperals Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, disableContentStyling will keep being internal implementation of an internal component. I think it makes more sense to cover the customer-facing components using the new props.

];

const permutations = createPermutations<SelectableItemProps>(permutationsConfigs);

export default function InputPermutations() {
return (
Expand Down
7 changes: 6 additions & 1 deletion src/internal/components/selectable-item/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const SelectableItem = (
isSelectAll,
virtualPosition,
padBottom,
disableContentStyling,
Copy link
Member

@jperals jperals Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need both approaches to remove the padding when the content is custom:

  • conditionally disable the padding
  • move the padding to an internal div
    Wouldn't one of them suffice?

Copy link
Member

@avinashbot avinashbot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - If all the "business" padding is now on this classname now, it seems easier to just remove the option-content classname if we want to disable the styling. Having a separate classname just to zero out the padding seems like too much work.

Copy link
Member Author

@SpyZzey SpyZzey Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good point, will think about it again

Copy link
Member Author

@SpyZzey SpyZzey Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, i don't know why i didn't think about that 😃
Option-content can't be removed as it was used for testing purposes before. But adding another with the padding and removing the class if disableContentStyling=true would make so much more sense.

isNextSelected,
isPreviousSelected,
useInteractiveGroups,
Expand Down Expand Up @@ -67,6 +68,10 @@ const SelectableItem = (
[styles['visual-refresh']]: isVisualRefresh,
});

const contentClassNames = clsx(styles['option-content'], analyticsSelectors['option-content'], {
[styles['selectable-item-content']]: !disableContentStyling,
});
Comment on lines +71 to +73
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new disableContentStyling property is not covered by unit tests in the __tests__ directory. Consider adding test cases to verify that the selectable-item-content class is conditionally applied based on this property, ensuring both true and false cases are tested.

Copilot uses AI. Check for mistakes.

const contentRef = useRef<HTMLDivElement>(null);
const screenReaderContentRef = useRef<HTMLDivElement>(null);

Expand Down Expand Up @@ -130,7 +135,7 @@ const SelectableItem = (
? {}
: getAnalyticsMetadataAttribute(getAnalyticsSelectActionMetadata({ isChild, value, ...restProps })))}
>
<div className={clsx(styles['option-content'], analyticsSelectors['option-content'])} ref={contentRef}>
<div className={contentClassNames} ref={contentRef}>
{content}
</div>
<div className={styles['measure-strut']} ref={ref} />
Expand Down
1 change: 1 addition & 0 deletions src/internal/components/selectable-item/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export type SelectableItemProps = BaseComponentProps & {
isSelectAll?: boolean;
virtualPosition?: number;
padBottom?: boolean;
disableContentStyling?: boolean;
isPreviousSelected?: boolean;
isNextSelected?: boolean;
useInteractiveGroups?: boolean;
Expand Down
106 changes: 76 additions & 30 deletions src/internal/components/selectable-item/styles.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,23 @@
border-inline-end-width: 0;
// to compensate for the loss of padding due to the removal of the left and right borders
// and differences in default divider + selected border widths (visual refresh)
padding-block: styles.$option-padding-with-border-placeholder-vertical;
padding-inline: calc(#{styles.$control-padding-horizontal} + #{awsui.$border-item-width});
padding-block: calc(#{awsui.$border-item-width} - #{awsui.$border-divider-list-width});
padding-inline: awsui.$border-item-width;

// For custom styles and background on the selectable-item-content, the content should not clip through the selectable-item borders..
overflow: hidden;

> .selectable-item-content {
padding-block: styles.$option-padding-vertical;
padding-inline: styles.$control-padding-horizontal;
}

&.pad-bottom {
padding-block-end: calc(#{styles.$option-padding-with-border-placeholder-vertical} + #{awsui.$space-xxxs});
padding-block-end: calc(#{awsui.$border-item-width} - #{awsui.$border-divider-list-width});
border-block-end-color: transparent;
> .selectable-item-content {
padding-block-end: calc(#{styles.$option-padding-vertical} + #{awsui.$space-xxxs});
}
}

&:not(:first-child),
Expand All @@ -42,6 +53,16 @@
&.has-background {
background-color: awsui.$color-background-dropdown-item-hover;
}
&.child {
padding-inline-start: awsui.$border-item-width;
> .selectable-item-content {
padding-inline-start: awsui.$space-xxl;
}
}

&.disabled > .selectable-item-content {
color: awsui.$color-text-dropdown-item-disabled;
}

&.highlighted,
&.selected {
Expand All @@ -51,22 +72,36 @@
border-start-end-radius: awsui.$border-radius-item;
border-end-start-radius: awsui.$border-radius-item;
border-end-end-radius: awsui.$border-radius-item;
padding-block: styles.$option-padding-vertical;
padding-inline: styles.$control-padding-horizontal;
padding-block: 0;
padding-inline: 0;

> .selectable-item-content {
padding-block: styles.$option-padding-vertical;
padding-inline: styles.$control-padding-horizontal;
}
&.child {
padding-inline-start: 0;
> .selectable-item-content {
padding-inline-start: awsui.$space-xxl;
}
}
&.pad-bottom {
padding-block-end: (calc(#{styles.$option-padding-vertical} + #{awsui.$space-xxxs}));
padding-block-end: 0;
> .selectable-item-content {
padding-block-end: (calc(#{styles.$option-padding-vertical} + #{awsui.$space-xxxs}));
}
}
}

&.highlighted {
z-index: 3;
background-color: awsui.$color-background-dropdown-item-hover;
border-color: awsui.$color-border-dropdown-item-hover;
&.disabled {
color: awsui.$color-text-dropdown-item-dimmed;
border-color: awsui.$color-border-dropdown-item-dimmed-hover;
background-color: awsui.$color-background-dropdown-item-dimmed;
> .selectable-item-content {
color: awsui.$color-text-dropdown-item-dimmed;
}
}
&.is-keyboard {
border-color: awsui.$color-border-dropdown-item-focused;
Expand Down Expand Up @@ -112,35 +147,47 @@
}

&.parent {
font-weight: bold;
color: awsui.$color-text-dropdown-group-label;
&:not(.disabled) {
> .selectable-item-content {
color: awsui.$color-text-dropdown-group-label;
}
}
&:not(.interactiveGroups) {
border-block-start-color: awsui.$color-border-dropdown-group;
padding-block: awsui.$space-xs;
padding-inline: awsui.$space-xs;
padding-block: 0;
padding-inline: 0;
&:not(:has(> .selectable-item-content)) {
padding-block: calc(#{awsui.$border-item-width} - #{awsui.$border-divider-list-width});
padding-inline: awsui.$border-item-width;
}

> .selectable-item-content {
padding-block: awsui.$space-xs;
padding-inline: awsui.$space-xs;
font-weight: bold;
}
}
&.interactiveGroups {
padding-block: styles.$group-option-padding-with-border-placeholder-vertical;
padding-inline: calc(#{styles.$control-padding-horizontal} + #{awsui.$border-item-width});
> .selectable-item-content {
padding-block: styles.$group-option-padding-vertical;
padding-inline: styles.$control-padding-horizontal;
}
&.highlighted {
color: awsui.$color-text-dropdown-item-highlighted;
> .selectable-item-content {
Copy link
Member

@jperals jperals Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: these two nested rulesets could be merged into one

&.highlighted > .selectable-item-content { ... }

color: awsui.$color-text-dropdown-item-highlighted;
}
}
&.highlighted,
&.selected {
padding-block: styles.$group-option-padding-vertical;
padding-inline: styles.$control-padding-horizontal;
padding-block: 0;
padding-inline: 0;
> .selectable-item-content {
padding-block: styles.$group-option-padding-vertical;
}
}
}
}

&.child {
padding-inline-start: calc(#{awsui.$space-xxl} + #{awsui.$border-item-width});
&.highlighted,
&.selected {
padding-inline-start: awsui.$space-xxl;
}
}

&.sticky {
position: sticky;
inset-block-start: 0;
Expand All @@ -156,7 +203,10 @@
border-inline-start-width: awsui.$border-item-width;
border-inline-start-color: awsui.$color-border-dropdown-container;
border-inline-end-color: awsui.$color-border-dropdown-container;
padding-inline: styles.$control-padding-horizontal;
padding-inline: 0;
> .selectable-item-content {
padding-inline: styles.$control-padding-horizontal;
}

&:not(.with-scrollbar) {
border-inline-end-width: awsui.$border-item-width;
Expand Down Expand Up @@ -187,10 +237,6 @@
}
}

&.disabled {
color: awsui.$color-text-dropdown-item-disabled;
}

&:not(.disabled):not(.parent) {
cursor: pointer;
}
Expand Down
Loading