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

[example] Make sure next.js Links can accept url objects as href #19073

Merged
merged 3 commits into from
Jan 4, 2020

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Jan 4, 2020

next.js links can accept parsed url objects, this PR makes sure the material ui wrapper in the example can accept these as well.

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 4, 2020

No bundle size changes comparing e170593...d0c4bc6

Generated by 🚫 dangerJS against d0c4bc6

@Janpot Janpot changed the title Make sure Links can accept url objects as href [example] Make sure next.js Links can accept url objects as href Jan 4, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: ready to ship labels Jan 4, 2020
@oliviertassinari oliviertassinari merged commit 4b6cbf0 into mui:master Jan 4, 2020
@oliviertassinari
Copy link
Member

@Janpot Thanks :).

I had a look at the table of content link issue, it doesn't seem that Next.js will prioritize a fix. If you want to apply the proposed patch in the issue you have open, I think that we could move forward with it.

@Janpot Janpot deleted the link-url-object branch January 4, 2020 19:42
@Janpot
Copy link
Member Author

Janpot commented Jan 4, 2020

@oliviertassinari I'm not 100% sure about the activeClass implementation in this example, I think it will behave differently whether you supply a string or a url object and href contains query parameters. Should the activeClass take query parameters into account?

regarding the hash links, I remember trying out the proposed fix and getting strange behavior, especially around trailing slashes. It seems like exportTrailingSlash is treated as a legacy option by the next.js team. Would it make sense to rather search for a way to remove this option from material-ui docs?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 4, 2020

Should the activeClass take query parameters into account?

I think that it should ignore query parameters.

Would it make sense to rather search for a way to remove this option from material-ui docs?

The biggest unknown is the SEO impact.

m4theushw pushed a commit to m4theushw/material-ui that referenced this pull request Jan 6, 2020
@Janpot
Copy link
Member Author

Janpot commented Jan 7, 2020

@oliviertassinari As long as pages with and without trailing slash either 301 redirect to the same url or have the same canonical url, the SEO impact should be none/minimal. In the meantime I opened vercel/next.js#9973

@oliviertassinari
Copy link
Member

Unless Google has some specific logic for handling trailing vs no trailing, introducting a 301 would impact ranking for a couple of months. We have already experienced it with v3 to v4 pathname renames.

@Janpot
Copy link
Member Author

Janpot commented Jan 7, 2020

Unless Google has some specific logic for handling trailing vs no trailing, introducting a 301 would impact ranking for a couple of months. We have already experienced it with v3 to v4 pathname renames.

I'd be surprised if they didn't already handle that specific case, but I don't want to introduce new redirects, the current situation 301 redirects non-trailing to trailing and that's good.

@oliviertassinari
Copy link
Member

OK, maybe we will be able to remove exportTrailingSlash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants