-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat(chips): Move logic for calculating chip bounding rect into a foundation method #4243
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4243 +/- ##
=========================================
Coverage ? 98.58%
=========================================
Files ? 127
Lines ? 5705
Branches ? 762
=========================================
Hits ? 5624
Misses ? 81
Partials ? 0
Continue to review full report at Codecov.
|
All 758 screenshot tests passed for commit eba9a1d vs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to add these to the Readme.
computeBoundingRect() { | ||
// When a chip has a checkmark and not a leading icon, the bounding rect changes in size depending on the current | ||
// size of the checkmark. | ||
if (!this.adapter_.hasLeadingIcon() && this.adapter_.getCheckmarkBoundingClientRect()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.adapter.getCheckmarkBoundingClientRect()
returns an object...I think it is more precise to check that the function call returns an object, instead of just calling the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to check to confirm it isn't null, does that work?
@@ -60,6 +60,9 @@ class MDCChipFoundation extends MDCFoundation { | |||
notifyRemoval: () => {}, | |||
getComputedStyleValue: () => {}, | |||
setStyleProperty: () => {}, | |||
hasLeadingIcon: () => {}, | |||
getBoundingClientRect: () => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding root
in the method makes it more clear. getRootBoundingClientRect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/mdc-chips/chip/index.js
Outdated
this.ripple_ = rippleFactory(this.root_); | ||
} | ||
const adapter = Object.assign(MDCRipple.createAdapter(this), { | ||
computeBoundingRect: () => this.foundation_.computeBoundingRect(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this.foundation_.computeBoundingRectForRipple()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to getDimensions
All 758 screenshot tests passed for commit 71ffdea vs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
All 758 screenshot tests passed for commit f0bdc5c vs. |
All 758 screenshot tests passed for commit f0bdc5c vs. |
This change makes adding ripple easier for clients wrapping chips.
BREAKING CHANGE: Adds 3 new chips adapter methods: hasLeadingIcon, getRootBoundingClientRect, and getCheckmarkBoundingClientRect. Also adds a new foundation method: getDimensions.