Skip to content

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