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(tabs): Allow tab hrefs to point anywhere #4860

Merged
merged 1 commit into from
Oct 25, 2016

Conversation

MitchLillie
Copy link
Contributor

  • Check for '#' before proceeding with querySelector and tab state modification
  • Allows anchors to point to other links

Closes #4072

* Check for '#' before proceeding with querySelector and tab state modification
* Allows anchors to point to other links

Closes google#4072
@sgomes
Copy link
Contributor

sgomes commented Oct 24, 2016

Thanks for the PR, @MitchLillie!

@Garbee Do you remember how we had decided to tackle this issue in v1? Does this PR work with that approach?

@Garbee
Copy link
Collaborator

Garbee commented Oct 24, 2016

Unless the tab js is affecting the layout js execution, this has been working as expected for 1.x.

Layout tabs can link to external resources, but internal page content tabs should not. As it is inconsistent if tabs are partially doing live-updating of the DOM and others cause a full page refresh.

This is only for 1.x's lifecycle, though, in 2.x this should change to allow for a comprehensive progressive-enhancement solution to be built by developers.

At this point, we should change in 2.x and as far as I can tell this is not a breaking change for 1.x. So, let's just go ahead and do it. ✔️

@sgomes
Copy link
Contributor

sgomes commented Oct 24, 2016

Sounds good, thanks @Garbee!

The change looks good to me, let's wait for a successful build and merge. Thanks again, @MitchLillie!

@MitchLillie
Copy link
Contributor Author

Thanks for the comments, all. The tabs in my case are being used with a php framework & router which is very opinionated about the way links are handled; no full-page refresh is actually triggered. Just confirming that this is allowing for an MD-legal use case. :)

What's up with the build @sgomes ? Anything blocking this on my end?

@sgomes
Copy link
Contributor

sgomes commented Oct 24, 2016

@MitchLillie Nope, your code is good :) It's just the CI acting up, again... :-/ I'll give this a spin locally when I get a chance instead.

@Garbee
Copy link
Collaborator

Garbee commented Oct 24, 2016

Restarting the build. Looks like a transient error with the memory usage tests.

@sgomes sgomes merged commit 40290b3 into google:mdl-1.x Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants