Skip to content

feat(material/chip): Add focus indicator #18019

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.

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
merged 3 commits into from
Jan 10, 2020
Merged

feat(material/chip): Add focus indicator #18019

merged 3 commits into from
Jan 10, 2020

Conversation

zelliott
Copy link
Collaborator

@zelliott zelliott commented Dec 20, 2019

Background: The chip's focus indicator needs to be offset from the chip itself in order to be sufficiently contrastive because the chip can have a primary/accent fill. However, given mat-chip has overflow: hidden, any offset focus indicator will be hidden. The reason mat-chip has its overflow hidden is because it also acts as a ripple target. Thus, in order to add a focus indicator to chip, this PR does the following:

  • Instead of using the mat-chip as the ripple target, dynamically create an element and add it as a child node of the mat-chip. This element will be the chip's new ripple target. This element will have its overflow hidden, and the mat-chip will have that style removed.
  • Add the .mat-focus-indicator class to the mat-chip.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 20, 2019
@zelliott zelliott marked this pull request as ready for review December 20, 2019 16:25
@zelliott
Copy link
Collaborator Author

Can I accept the new api golden?

@zelliott zelliott requested a review from crisbeto December 21, 2019 15:35
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM


// Ensures that the ripple effect doesn't overflow the ripple target.
overflow: hidden;
}
Copy link
Member

Choose a reason for hiding this comment

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

If only all CSS was commented this well.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 10, 2020
@ngbot
Copy link

ngbot bot commented Jan 10, 2020

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: api_golden_checks" is failing
    pending missing required labels: target: *

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn jelbourn removed the action: merge The PR is ready for merge by the caretaker label Jan 10, 2020
@jelbourn
Copy link
Member

You can go ahead and make the public API gold update and I can merge the PR

@mmalerba mmalerba merged commit 172ee1d into angular:focus-indicator Jan 10, 2020
mmalerba pushed a commit that referenced this pull request Jan 21, 2020
* Add focus indicators to mat-chip. Use a dynamically added element as the ripple target

* _document should be an optional param to avoid breaking change.

* Updated chips API golden.
jelbourn pushed a commit that referenced this pull request Jan 28, 2020
* Add focus indicators to mat-chip. Use a dynamically added element as the ripple target

* _document should be an optional param to avoid breaking change.

* Updated chips API golden.
mmalerba pushed a commit that referenced this pull request Feb 4, 2020
* Add focus indicators to mat-chip. Use a dynamically added element as the ripple target

* _document should be an optional param to avoid breaking change.

* Updated chips API golden.
mmalerba pushed a commit that referenced this pull request Feb 4, 2020
* Add focus indicators to mat-chip. Use a dynamically added element as the ripple target

* _document should be an optional param to avoid breaking change.

* Updated chips API golden.
@angular-automatic-lock-bot
Copy link

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 Feb 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants