Skip to content

fix(material/list): add radio toggles#25933

Merged
zarend merged 1 commit intoangular:mainfrom
zarend:mdc-list-radio
Nov 22, 2022
Merged

fix(material/list): add radio toggles#25933
zarend merged 1 commit intoangular:mainfrom
zarend:mdc-list-radio

Conversation

@zarend
Copy link
Copy Markdown
Contributor

@zarend zarend commented Nov 7, 2022 •
edited
Loading

fix(material/list): add radio toggles

Add radio toggles for single selection. Fix a11y issue where selected
state is visually communicated with color alone.

Rename checkboxPosition Input to togglePosition and deprecate
checkboxPosition. togglePosition configures the position of both the
radio and checkbox indicators. checkboxPosition also configures the
position of both.

Summary of API and behavior changes:

  • MDC List displays radio indicators for single-selection
  • rename checkboxPosition Input to togglePosition
  • rename type MatListOptionCheckboxPosition to type MatListOptionTogglePosition

DEPRECTED:

  • checkboxPosition is deprecated because togglePosition replaces it
  • MatListOptionCheckboxPosition is deprecated because
    MatListOptionTogglePosition replaces it

Closes #7157, Fixes #25900

@zarend zarend added Accessibility This issue is related to accessibility (a11y) area: material/list target: rc This PR is targeted for the next release-candidate labels Nov 7, 2022
@zarend zarend mentioned this pull request Nov 7, 2022
Closed
@zarend zarend force-pushed the mdc-list-radio branch 2 times, most recently from 2683a4f to 18ea893 Compare November 7, 2022 17:34
mmalerba
mmalerba approved these changes Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

devversion
devversion reviewed Nov 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM, nice change!

crisbeto
crisbeto reviewed Nov 8, 2022
View reviewed changes
@zarend zarend force-pushed the mdc-list-radio branch 6 times, most recently from 8e892c4 to 8072226 Compare November 9, 2022 04:01
@zarend
Copy link
Copy Markdown
Contributor Author

zarend commented Nov 9, 2022

Hi all, I've made fixes to respond to PR comments

  • update the code comments: say checkbox/radio or toggle instead of just checkbox
  • delete MatListOptionCheckboxPosition and @Input checkboxPosition, which are replaced by MatListOptionTogglePosition and @Input togglePosition
  • add getRadioPosition method to test harness

@crisbeto does this PR look good to you?

crisbeto
crisbeto reviewed Nov 9, 2022
View reviewed changes
@zarend zarend added target: minor This PR is targeted for the next minor release and removed target: rc This PR is targeted for the next release-candidate labels Nov 9, 2022
@zarend
Copy link
Copy Markdown
Contributor Author

zarend commented Nov 9, 2022

I've made fixed based on PR comments and changed the target to minor:

  • checkboxPosition is deprecated because togglePosition replaces it
  • MatListOptionCheckboxPosition is deprecated because MatListOptionTogglePosition replaces it
  • export MatListOptionCheckboxPosition using the export as syntax 👌

@crisbeto @mmalerba this is ready for your eyes again 👀

@zarend zarend requested review from crisbeto and mmalerba November 9, 2022 21:55
crisbeto
crisbeto approved these changes Nov 9, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Nov 9, 2022
@zarend zarend removed the action: merge The PR is ready for merge by the caretaker label Nov 17, 2022
@zarend
fix(material/list): add radio toggles
eccc4a8 
Add radio toggles for single selection. Fix a11y issue where selected
state is visually communicated with color alone.

Rename `checkboxPosition` Input to `togglePosition` and deprecate
`checkboxPosition`. `togglePosition` configures the position of both the
radio and checkbox indicators. `checkboxPosition` also configures the
position of both.

Summary of API and behavior changes:
 - MDC List displays radio indicators for single-selection
 - rename `checkboxPosition` Input to `togglePosition`
 - rename `type MatListOptionCheckboxPosition` to `type
   MatListOptionTogglePosition`

DEPRECTED:
 * `checkboxPosition` is deprecated because `togglePosition` replaces it
 * `MatListOptionCheckboxPosition` is deprecated because
   `MatListOptionTogglePosition` replaces it

Closes angular#7157, Fixes angular#25900
@zarend
Copy link
Copy Markdown
Contributor Author

zarend commented Nov 17, 2022

changes since this was last reviewed:

  • fixed regression in radio dark mode because MDC's global variables were not configured correctly
  • consolidated calls to using-mdc-theme

@zarend zarend added the action: merge The PR is ready for merge by the caretaker label Nov 22, 2022
@zarend zarend merged commit 57676e4 into angular:main Nov 22, 2022
@zarend zarend mentioned this pull request Dec 12, 2022
Closed
zarend added a commit to zarend/components that referenced this pull request Dec 16, 2022
@zarend
fix(material/list): add opt-out for radio indicators
dbab436 
Add an opt-out for radio indicators for single-selection. Adds both an
Input and DI token to specify if radio indicators are hidden. By
default, display radio indicators. If both DI token and Input are
specified, the Input wins.

PR angular#25933 added radio toggles for single-selection. Add an opt-out to
provide a way to have same appearance as before angular#25933.

API changes
 - add `@Input hideSingleSelectionIndicator` to specify if radio
   indicators are displayed
 - add MAT_LIST_CONFIG token to specify default value for
   `hideSingleSelectionIndicator`
@zarend zarend mentioned this pull request Dec 16, 2022
Merged
zarend added a commit to zarend/components that referenced this pull request Dec 16, 2022
@zarend
fix(material/list): add opt-out for radio indicators
11334df 
Add an opt-out for radio indicators for single-selection. Adds both an
Input and DI token to specify if radio indicators are hidden. By
default, display radio indicators. If both DI token and Input are
specified, the Input wins.

PR angular#25933 added radio toggles for single-selection. Add an opt-out to
provide a way to have same appearance as before angular#25933.

API changes
 - add `@Input hideSingleSelectionIndicator` to specify if radio
   indicators are displayed
 - add MAT_LIST_CONFIG token to specify default value for
   `hideSingleSelectionIndicator`
zarend added a commit that referenced this pull request Dec 20, 2022
@zarend
fix(material/list): add opt-out for radio indicators (#26283)
25d9217 
Add an opt-out for radio indicators for single-selection. Adds both an
Input and DI token to specify if radio indicators are hidden. By
default, display radio indicators. If both DI token and Input are
specified, the Input wins.

PR #25933 added radio toggles for single-selection. Add an opt-out to
provide a way to have same appearance as before #25933.

API changes
 - add `@Input hideSingleSelectionIndicator` to specify if radio
   indicators are displayed
 - add MAT_LIST_CONFIG token to specify default value for
   `hideSingleSelectionIndicator`
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Dec 23, 2022

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Dec 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@devversion devversion devversion left review comments

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

+1 more reviewer

@mmalerba mmalerba mmalerba approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Accessibility This issue is related to accessibility (a11y) action: merge The PR is ready for merge by the caretaker area: material/list target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(material/list): selected state is visually communicated only with color (WCAG 1.4.1) [MdSelectionList] single selection (radio) option

4 participants

@zarend @crisbeto @devversion @mmalerba