-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(framework-examples): Add ripple support to React checkbox example #233
feat(framework-examples): Add ripple support to React checkbox example #233
Conversation
This will only work if the newest checkbox.css is available.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
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.
Thanks for taking the initiative on this! Few changes needed but overall looks good.
@@ -47,11 +57,10 @@ export default class Checkbox extends PureComponent { | |||
|
|||
state = { | |||
classes: new ImmutableSet(), | |||
css: new ImmutableMap(), |
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.
perhaps it would be better to call this rippleCss?
// For browser compatibility we extend the default adapter which checks for css variable support. | ||
rippleFoundation = new MDCRippleFoundation(Object.assign(MDCRipple.createAdapter(this), { | ||
isUnbounded: () => true, | ||
isSurfaceActive: () => this.refs.root[MATCHES](':active'), |
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 you actually want to check is that the input element is active, rather than the root. See #241
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.
Makes sense! I was wondering why it's the root. I was using the native js version that you fixed as my guide. Thanks for clearing that up.
Thanks for the feedback! I'll have a look in about 40 minutes. I'll also reply to the other pullreq seems I messed something up there. |
Fixed! I have a question about ripple. Maybe you can clear something up for me. I figured out that the classes and CSS need to be set on the DOM element carrying the specific class like
Here ripple needs a ref of button and also needs to be able to set buttons I tried using Question 1: Question 2: For now I found it best to just implement the ripple inside the specific class like here: What do you think? |
@codesuki regarding your question, You're totally right in that the ripple should be a part of a component, not something completely different. E.g. with buttons, a button component would manage a ripple as part of the component itself. This is how we approach it within MDC-Web, although for our components, we simply instruct users to attach a ripple to buttons. The reason for this is that our vanilla buttons require no javascript, so we didn't want to introduce a wrapper component simply to facilitate a ripple For frameworks like React, JS is always required no matter what the component since it takes care of rendering the component DOM for the end user. In this case, you'll definitely want to manage the ripple within those components, as you've suggested. Perhaps using a higher-order function that creates a component with ripple capabilities would be a sound strategy? |
Thanks for clearing that up! Glad I am on the same page. |
This will only work if the newest checkbox.css is available. So maybe should be merged after a new checkbox version is released (0.1.3?).
Also
getMatchesProperty
is not exported from Ripple. I might send another pull request since it looks very useful for implementing new components.