Skip to content
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

Merged
merged 24 commits into from Jul 25, 2016
Merged

feat(ripple): initial mdInkRipple implementation #681

merged 24 commits into from Jul 25, 2016

Conversation

dozingcat
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 14, 2016
bottom: 0;
}

.md-ripple-background.active {
Copy link
Member

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

Copy link
Contributor Author

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,
Copy link
Member

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.

Copy link
Contributor Author

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.

@jelbourn
Copy link
Member

jelbourn commented Jul 6, 2016

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

complets --> completes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kara
Copy link
Contributor

kara commented Jul 6, 2016

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(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of 'window.requestAnimationFrame()' like tab component when update ink-bar?
@jelbourn @kara

@paralin
Copy link

paralin commented Jul 8, 2016

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.
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

@jelbourn
Copy link
Member

LGTM aside from the removing of "ink".

@jelbourn jelbourn added the action: merge The PR is ready for merge by the caretaker label Jul 22, 2016
@hansl hansl merged commit 47448cb into angular:master Jul 25, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants