-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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): initial mdInkRipple implementation #681
Conversation
bottom: 0; | ||
} | ||
|
||
.md-ripple-background.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.
.active
should be something like .md-ripple-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.
done
styleUrls: ['ripple-demo.css'], | ||
providers: [MdUniqueSelectionDispatcher], | ||
directives: [ | ||
MdButton, MdCard, MdCheckbox, MdIcon, MdInput, MdRadioButton, MdRadioGroup, MdInkRipple, |
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.
Can you switch these to the MD_XXX_DIRECTIVES constants? We're trying to push for people to always use those.
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, and added MD_RIPPLE_COMPONENTS.
LGTM aside from a few minor comments. It would be good if one of @robertmesserle, @kara, or @hansl could also do a pass, though. |
|
||
`md-ink-ripple` defines an area in which a ripple animates, usually in response to user action. | ||
|
||
By default, an `md-ink-ripple` component is activated when its parent element receives mouse or touch events. On a mousedown or touch start, the ripple background fades in. When the click event complets, a circular foreground ripple fades in and expands from the event location to cover the component bounds. |
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.
complets --> completes
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
Found a few typos and the alpha version needs correcting, but once those are fixed, LGTM. |
* the "centered" property is set or forceCenter is true). | ||
*/ | ||
end(left: number, top: number, forceCenter = true) { | ||
this._rippleManager.createForegroundRipple( |
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.
…cat/material2 into mdInkRipple_ComponentOnly
Looking forward to this being merged. |
| `md-ink-ripple-max-radius` | number | Optional fixed radius of foreground ripples when fully expanded. Mainly used in conjunction with `unbounded` attribute. If not set, ripples will expand from their origin to the most distant corner of the component's bounding rectangle. | ||
| `md-ink-ripple-unbounded` | boolean | If true, foreground ripples will be visible outside the component's bounds. | ||
| `md-ink-ripple-focused` | boolean | If true, the background ripple is shown using the current theme's accent color to indicate focus. | ||
| `md-ink-ripple-disabled` | boolean | If true, click events on the trigger element will not activate ripples. The `start` and `end` methods can still be called to programmatically create ripples. |
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 do you think of omitting ink-
and just having md-ripple
?
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.
Seems reasonable. Rename the classes to MdRipple as well?
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.
Yeah, let's do that.
LGTM aside from the removing of "ink". |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.