feat(tabs): add ripples to the tab group and nav bar links #1700
Conversation
googlebot
added
the
cla: yes
| @@ -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.
Does the user have to add the ripple themselves if using tab-link?
There was a problem hiding this comment.
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.
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.
andrewseguin
changed the title
|
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.
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.
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.
andrewseguin
force-pushed
the
tabs-ripple
branch
from
cc72777
to
f88baad
Compare
|
@andrewseguin can you rebase? |
andrewseguin
force-pushed
the
tabs-ripple
branch
from
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.