Skip to content

fix(material-experimental/button): high contrast outline for solitary icon-buttons - #22987

Merged
mmalerba merged 1 commit into
angular:masterfrom
jelbourn:icon-high-contrast
Jul 23, 2021
Merged

fix(material-experimental/button): high contrast outline for solitary icon-buttons#22987
mmalerba merged 1 commit into
angular:masterfrom
jelbourn:icon-high-contrast

Conversation

@jelbourn

@jelbourn jelbourn commented Jun 15, 2021

Copy link
Copy Markdown
Contributor

The high-contrast style for all buttons was previously defined in
button.scss, but icon-buttons load icon-button.scss. This means that
the high contrast style for icon buttons was only loaded when another
type of button was on the page. This change moved the icon-button high-contrast
style to icon-button.scss.

cc @zarend @amysorto FYI

@jelbourn jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent Accessibility This issue is related to accessibility (a11y) area: material/button labels Jun 15, 2021
@jelbourn
jelbourn requested a review from crisbeto June 15, 2021 20:32
@jelbourn
jelbourn requested a review from andrewseguin as a code owner June 15, 2021 20:32
@google-cla google-cla Bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 15, 2021
crisbeto
crisbeto reviewed Jun 15, 2021
View reviewed changes
Comment thread src/material-experimental/mdc-button/icon-button.scss Outdated
}
}

@include a11y.high-contrast(active, off) {

@crisbeto crisbeto Jun 15, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The non-icon button also increases the outline to 3px on focus. Should we have the same here? It might be a good idea to pull these styles into _button-base.scss so they can be reused.

@jelbourn jelbourn Jun 21, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the PR to move the HC styles into their own file that's imported by both button and icon-button (but not fab)

@jelbourn
jelbourn force-pushed the icon-high-contrast branch 4 times, most recently from 66b17b7 to 3481f7f Compare June 21, 2021 20:20
@jelbourn
jelbourn force-pushed the icon-high-contrast branch from 3481f7f to 7bac83a Compare July 2, 2021 20:53
@jelbourn

jelbourn commented Jul 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@crisbeto PTAL at the updated changes

crisbeto
crisbeto approved these changes Jul 2, 2021
View reviewed changes

@crisbeto crisbeto left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 2, 2021
@jelbourn jelbourn removed the action: merge The PR is ready for merge by the caretaker label Jul 2, 2021
@jelbourn
fix(material-experimental/mdc-button): high contrast outline for soli…
fb2f0da 
…tary

icon-buttons

The high-contrast style for all buttons was previously defined in
`button.scss`, but icon-buttons load `icon-button.scss`. This means that
the high contrast style for icon buttons was only loaded when another
type of button was on the page. This change moves the high-contrast styles to their own file that's referenced from buttons and icon-buttons (but not FABs).
@jelbourn
jelbourn force-pushed the icon-high-contrast branch from 7bac83a to fb2f0da Compare July 2, 2021 21:59
@jelbourn

jelbourn commented Jul 2, 2021

Copy link
Copy Markdown
Contributor Author

Whoops, I accidentally pushed a stale branch somehow. Updated with the real changes

@jelbourn jelbourn added the target: patch This PR is targeted for the next patch release label Jul 7, 2021
@andrewseguin

andrewseguin commented Jul 15, 2021

Copy link
Copy Markdown
Contributor

@jelbourn Did you want to add the merge ready label back on

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jul 21, 2021
@jelbourn

jelbourn commented Jul 21, 2021

Copy link
Copy Markdown
Contributor Author

Added it back

@mmalerba
mmalerba merged commit 30e9205 into angular:master Jul 23, 2021
mmalerba pushed a commit that referenced this pull request Jul 23, 2021
@jelbourn @mmalerba
fix(material-experimental/mdc-button): high contrast outline for soli…
762140a 
…tary (#22987)

icon-buttons

The high-contrast style for all buttons was previously defined in
`button.scss`, but icon-buttons load `icon-button.scss`. This means that
the high contrast style for icon buttons was only loaded when another
type of button was on the page. This change moves the high-contrast styles to their own file that's referenced from buttons and icon-buttons (but not FABs).

(cherry picked from commit 30e9205)
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Aug 23, 2021

Copy link
Copy Markdown

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 Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes
@andrewseguin andrewseguin Awaiting requested review from andrewseguin
+1 more reviewer
@zarend zarend zarend 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/button cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jelbourn @andrewseguin @crisbeto @zarend @mmalerba