-
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(ripple): Implement subset of improved interaction response guide… #363
feat(ripple): Implement subset of improved interaction response guide… #363
Conversation
148a0ec
to
0947879
Compare
3e110bf
to
d65cf1b
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.
I think that these changes need some tests to go along with them to cover the new logic introduced in the source file
@@ -146,12 +148,20 @@ export default class MDCRippleFoundation extends MDCFoundation { | |||
|
|||
activationState.isActivated = true; | |||
activationState.isProgrammatic = e === null; | |||
activationState.isTimeoutDeactivated = false; |
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 this, as well as isProgrammatic
, need to be added to the return value of defaultActivationState_()
packages/mdc-ripple/foundation.js
Outdated
activationState.activationEvent = e; | ||
activationState.wasActivatedByPointer = activationState.isProgrammatic ? false : ( | ||
e.type === 'mousedown' || e.type === 'touchstart' || e.type === 'pointerdown' | ||
); | ||
|
||
activationState.activationStartTime = Date.now(); | ||
|
||
activationState.deactivationTimeout = setTimeout(() => { |
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.
same comment as above
…lines for ripple Resolves #190
d65cf1b
to
7eceb51
Compare
All issues addressed PTAL |
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.
Will there be any updates to the unbounded ripple interaction? I don't see any changes to its UX, nor do I see any tests for the unbounded deactivation classes.
@@ -101,6 +101,43 @@ testFoundation('runs deactivation UX on public deactivate() call', ({foundation, | |||
td.verify(adapter.addClass(cssClasses.FG_BOUNDED_ACTIVE_FILL)); | |||
}); | |||
|
|||
testFoundation('runs deactivation UX mousedown pressed for longer than deactivation timeout', |
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.
so for both of these, the tests will actually exit before the setTimeout()
code is executed. This is because the tests execute synchronously, so by the time that setTimeout
code runs, the test will be finished and any exceptions will be swallowed.
To prevent this, you can use lolex, which we use in our codebase to mock JS timers. You can see examples of this in the following tests:
mockRaf.flush(); | ||
|
||
setTimeout(() => { | ||
td.verify(adapter.removeClass(cssClasses.BG_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.
Not exactly sure how these tests are different from the ones above...
Should this be verifying that these adapter methods are not called before DEACTIVATION_TIMEOUT_MS
?
All issues addressed PTAL |
packages/mdc-ripple/constants.js
Outdated
@@ -53,4 +53,5 @@ export const numbers = { | |||
UNBOUNDED_TRANSFORM_DURATION_MS: 200, | |||
PADDING: 10, | |||
INITIAL_ORIGIN_SCALE: 0.6, | |||
DEACTIVATION_TIMEOUT_MSL: 200, |
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.
looks like this should be named DEACTIVATION_TIMEOUT_MS
instead of DEACTIVATION_TIMEOUT_MSL
…lines for ripple Resolves #190
The "Make Indeterminate" button in the JS checkbox demo is currently not working because the "indeterminate" property is being set on the checkbox _root_ element, rather than the native instance. This commit fixes that. Skipping TravisCI as this is a change to the demo only. [ci skip]
- Update scripts/pre-release.sh with correct publish command - Update CONTRIBUTING.md releasing section with guidance on what to do when a release goes awry. [ci skip]
…lines for ripple Resolves #190
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
feat(ripple): Implement subset of improved interaction response guidelines for ripple
Resolves #190