Skip to content

Commit

Permalink
fix(material/list): align color scheme between single and multi selec…
Browse files Browse the repository at this point in the history
…tion list

MDC has the `mdc-list-item--selected` class which we were setting only on single selection `mat-list-option` to indicate using color that they're selected. After the recent change to indicate selection using a radio button, these look weird because the radio button inherits the list item theme palette while the background of the item stays as `primary`.

These changes resolve the issue by treating single selection and multi-selection lists the same.

**Note:** an alternative I was considering was to change the color so that it matches the radio button color, but then the appearance of the single selection list would be inconsistent with the multi-selection list.
  • Loading branch information
crisbeto committed Dec 3, 2022
1 parent 962c9a3 commit e720d89
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 25 deletions.
17 changes: 0 additions & 17 deletions src/material/list/_interactive-list-theme.scss
@@ -1,13 +1,11 @@
@use 'sass:map';
@use '@material/ripple' as mdc-ripple;
@use '../core/theming/theming';

// Mixin that provides colors for the various states of an interactive list-item. MDC
// has integrated styles for these states but relies on their complex ripples for it.
@mixin private-interactive-list-item-state-colors($config) {
$is-dark-theme: map.get($config, is-dark);
$active-base-color: if($is-dark-theme, white, black);
$selected-color: theming.get-color-from-palette(map.get($config, primary));

.mat-mdc-list-item-interactive {
&::before {
Expand All @@ -18,21 +16,6 @@
opacity: mdc-ripple.states-opacity($active-base-color, hover);
}

&.mdc-list-item--selected {
&::before {
background: $selected-color;
opacity: mdc-ripple.states-opacity($selected-color, selected);
}

&:not(:focus):not(.mdc-list-item--disabled):hover::before {
// The hover and selected opacities need to be combined to match with what the MDC
// ripple state would render. More details here:
// https://github.com/material-components/material-components-web/blob/348665978ce73694ad4518626dd70cdf5b984113/packages/mdc-ripple/_ripple-theme.scss#L450.
opacity: mdc-ripple.states-opacity($selected-color, hover) +
mdc-ripple.states-opacity($selected-color, selected);
}
}

&:focus::before {
opacity: mdc-ripple.states-opacity($active-base-color, focus);
}
Expand Down
3 changes: 0 additions & 3 deletions src/material/list/list-option.ts
Expand Up @@ -64,9 +64,6 @@ export interface SelectionList extends MatListBase {
host: {
'class': 'mat-mdc-list-item mat-mdc-list-option mdc-list-item',
'role': 'option',
// As per MDC, only list items in single selection mode should receive the `--selected`
// class. For multi selection, the checkbox is used as indicator.
'[class.mdc-list-item--selected]': 'selected && !_selectionList.multiple',
// Based on the checkbox/radio position and whether there are icons or avatars, we apply MDC's
// list-item `--leading` and `--trailing` classes.
'[class.mdc-list-item--with-leading-avatar]': '_hasProjected("avatars", "before")',
Expand Down
8 changes: 3 additions & 5 deletions src/material/list/selection-list.spec.ts
Expand Up @@ -1041,16 +1041,14 @@ describe('MDC-based MatSelectionList without forms', () => {
fixture.detectChanges();

expect(selectList.selected).toEqual([testListItem1]);
expect(listOptions[1].nativeElement.classList.contains('mdc-list-item--selected')).toBe(true);
expect(listOptions[1].nativeElement.getAttribute('aria-selected')).toBe('true');

dispatchMouseEvent(testListItem2._hostElement, 'click');
fixture.detectChanges();

expect(selectList.selected).toEqual([testListItem2]);
expect(listOptions[1].nativeElement.classList.contains('mdc-list-item--selected')).toBe(
false,
);
expect(listOptions[2].nativeElement.classList.contains('mdc-list-item--selected')).toBe(true);
expect(listOptions[1].nativeElement.getAttribute('aria-selected')).toBe('false');
expect(listOptions[2].nativeElement.getAttribute('aria-selected')).toBe('true');
});

it('should not show check boxes', () => {
Expand Down

0 comments on commit e720d89

Please sign in to comment.