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(ripple): Add createAdapter() static method #4755
Conversation
b239b17
to
ea58581
Compare
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.
Lg. Just needs the change on "specifying known element dimensions".
```js | ||
this.ripple_ = new MDLRippleFoundation({ | ||
// ... | ||
computeBoundingRect: () => Object.assign({}, element.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.
My first thought was that this would yield incorrect right
and bottom
values in the resulting rect, but after testing it out, it looks like ClientRect's fields are not enumerable, so you end up with { width: 24, height: 24 }
.
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.
ugh lame. But great catch thank you.
@@ -32,6 +32,23 @@ export default class MDLRipple extends MDLComponent { | |||
return ripple; | |||
} | |||
|
|||
static createAdapter(instance) { | |||
return { | |||
browserSupportsCssVars: () => supportsCssVariables(window), |
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 see this was just a move, but to satisfy my curiosity:
Under what circumstances would a component using MDLRippleFoundation
want to override this?
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.
Since we (pretty much) can't make any assumptions about our runtime environment within the foundation, the adapter needs a method that lets it feature-detect weather or not the browser supports the correct functionality. Furthermore, clients - especially ones that use libraries / frameworks - can use their own feature detection library if need be. Also we explicitly put this into an exported function within ./utils
so clients can just reuse ours if so desired, and I believe we actually suggest using it within our README.
@@ -51,6 +51,13 @@ test('attachTo overrides unbounded data attr when explicitly specified', t => { | |||
t.end(); | |||
}); | |||
|
|||
test('createAdapter() returns the same adapter used by default for the ripple', t => { | |||
const root = bel`<div></div>`; |
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.
Ohhh...bel is nice. :)
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.
oh yeah. I came across it via yo-yo, also really nice.
createAdapter() easily lets other MDLComponents create ripple adapters that they can then extend to their needs. Updated the README with some tips/tricks on where this could be useful. Note that the icon toggle has an immediate need for this functionality.
ea58581
to
5eb7f2b
Compare
@shyndman changes made PTAL |
createAdapter() easily lets other MDLComponents create ripple adapters that they can then extend to their needs. Updated the README with some tips/tricks on where this could be useful. Note that the icon toggle has an immediate need for this functionality.