-
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(tabs): add ripples to the tab group and nav bar links #1700
Conversation
@@ -2,7 +2,7 @@ | |||
|
|||
<div class="demo-nav-bar"> | |||
<nav md-tab-nav-bar aria-label="weather navigation links"> | |||
<a md-tab-link | |||
<a md-tab-link 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.
Does the user have to add the ripple themselves if using tab-link?
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, this isn't so ideal. I talked to Jeremy and I might be able to play around a bit more with md-tab-link to get it working as a ripple. I'll send a message when this PR has that incorporated
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.
Sure, sounds good. If you haven't already, you might want to check out md-button's ripple. It's also an attr directive on an anchor.
Okay, I made MdTabLink extend the MdRipple so that it automatically applies the ripple effect on the element. What do you guys think of this approach? |
Tests are failing due to lacking changes from PR #1684 which fixes ripple pointer events |
Tests are passing, able to be reviewed now |
@@ -24,7 +25,7 @@ export class MdTabNavBar { | |||
@Directive({ | |||
selector: '[md-tab-link]', | |||
}) | |||
export class MdTabLink { | |||
export class MdTabLink extends MdRipple { |
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.
Conceptually, I find it a bit confusing that the link extends a ripple. Any reason why we wouldn't convert this to a component and use the md-ripple-trigger property?
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 agree, it's odd to say that the md tab link extends a ripple. I'd prefer to keep this an attribute directive to preserve the end user's HTML using all native elements (nav, a), e.g.:
<nav md-tab-nav-bar>
<a md-tab-link>Link</a>
<a md-tab-link>Link</a>
</nav>
So there are two more alternatives:
- Add [md-tab-link] selector to the MdRipple
- Create a new MdTabLinkRipple directive that will only extend MdRipple and match [md-tab-link]
In talking with Jeremy, I think (2) sounds better than MdTabLink extending MdRipple and (1) just sounds more silly than either.
cc72777
to
f88baad
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.
Ripple has been extracted out of the md tab link
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.
LGTM
@andrewseguin can you rebase? |
f88baad
to
2335dc8
Compare
Conflicts resolved, thanks |
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.