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

Fix paper tab disabled #971

Merged

Conversation

MarcoUmpierrez
Copy link
Contributor

Hi, I saw that this issue #791 hasn't been fixed yet. Here's a PR to fix it. I've added a test and an example of a disabled tab in the dummy app.

@@ -57,7 +57,9 @@ export default Component.extend(ChildMixin, RippleMixin, FocusableMixin, {
},

click() {
this.sendAction('onClick', ...arguments);
this.sendAction('onSelect', this);
if(!this.get("disabled")) {
Copy link
Owner

Choose a reason for hiding this comment

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

@MarcoUmpierrez could you please change this line to if (!this.get('disabled')) { to conform with the rest of the project's coding style?

@miguelcobain
Copy link
Owner

miguelcobain commented Aug 29, 2018

@MarcoUmpierrez this looks awesome. Thank you so much.

There's still something that isn't covered that shouldn't be too hard to do.

A tab can be an anchor if an href is passed. If a tab with an href is disabled, we need to disable a link. But how do we do that?
Well, a simple way is to just don't have any href.
So we can have an intermediate value that "maybe" holds the href if not disabled.

attributeBindings: ['maybeHrefhref'],
maybeHref: computed('href', 'disabled', function() {
  if (this.get('href') && !this.get('disabled')) {
    return this.get('href');
  }
}),

Having a test that made sure that no href was rendered for a disabled tab would also be helpful.

@MarcoUmpierrez
Copy link
Contributor Author

@miguelcobain Added the changes you requested and a test to check if the href is set when the tab is disabled.

@miguelcobain miguelcobain merged commit 4da7dd0 into miguelcobain:master Aug 30, 2018
@miguelcobain
Copy link
Owner

Thanks a lot @MarcoUmpierrez !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants