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

[Docs] wrong url when cmd+click on right side navigation #18756

Closed
2 tasks done
Janpot opened this issue Dec 9, 2019 · 7 comments · Fixed by #19097
Closed
2 tasks done

[Docs] wrong url when cmd+click on right side navigation #18756

Janpot opened this issue Dec 9, 2019 · 7 comments · Fixed by #19097
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@Janpot
Copy link
Member

Janpot commented Dec 9, 2019

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

First of all, this is an extremely minor issue, but just so you're aware:

It happens in the right-side navigation menu for components. When I cmd+click an item to open it in another tab, e.g. on 'Demos' it goes to https://material-ui.com/#demos, which redirects to https://v0.material-ui.com/#/demos

Expected Behavior 🤔

It should open the correct page in a new tab and the page should scroll to the selected section.

Steps to Reproduce 🕹

Steps:

  1. go to https://material-ui.com/components/box/
  2. cmd+click on an item in the ride-side navigation
  3. look at the tab it opens

Context 🔦

Your Environment 🌎

Tech Version
Material-UI v4.7.2
React
Browser latest chrome
TypeScript
etc.
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work docs Improvements or additions to the documentation labels Dec 9, 2019
@oliviertassinari
Copy link
Member

@Janpot Thank you for reporting this bug. Would this be enough?

diff --git a/docs/src/modules/components/AppTableOfContents.js b/docs/src/modules/components/AppTableOfContents.js
index e32069e18..31280966d 100644
--- a/docs/src/modules/components/AppTableOfContents.js
+++ b/docs/src/modules/components/AppTableOfContents.js
@@ -11,6 +11,7 @@ import Typography from '@material-ui/core/Typography';
 import textToHash from 'docs/src/modules/utils/textToHash';
 import DiamondSponsors from 'docs/src/modules/components/DiamondSponsors';
 import Link from 'docs/src/modules/components/Link';
+import PageContext from 'docs/src/modules/components/PageContext';

 const useStyles = makeStyles(theme => ({
   root: {
@@ -155,6 +156,8 @@ export default function AppTableOfContents(props) {
     itemsClientRef.current = getItemsClient(itemsServer);
   }, [itemsServer]);

+  const { activePage } = React.useContext(PageContext);
+
   const [activeState, setActiveState] = React.useState(null);
   const clickedRef = React.useRef(false);
   const unsetClickedRef = React.useRef(null);
@@ -221,7 +224,7 @@ export default function AppTableOfContents(props) {
     <Link
       display="block"
       color={activeState === item.hash ? 'textPrimary' : 'textSecondary'}
-      href={`#${item.hash}`}
+      href={`${activePage.pathname}/#${item.hash}`}
       underline="none"
       onClick={handleClick(item.hash)}
       className={clsx(

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Dec 9, 2019
@Janpot
Copy link
Member Author

Janpot commented Dec 9, 2019

@oliviertassinari It doesn't seem to reproduce when I run the docs locally. I'm wondering if this is a next.js issue with the production build

edit:

It looks like cmd+click also activates the item in the side-navigation without scrolling to it

@oliviertassinari
Copy link
Member

Cool, do you want to take care of the hash issue at the same time in #18765?

@Janpot
Copy link
Member Author

Janpot commented Dec 9, 2019

I'm not yet sure what causes it. Your proposal would probably fix it, but I feel like there's another bug at play here since it's supposed to work the way it is built now.

@Janpot
Copy link
Member Author

Janpot commented Dec 10, 2019

@oliviertassinari I filed vercel/next.js#9678

@oliviertassinari
Copy link
Member

Thanks for the bug report. Note that we are still using Next.js v8.1.0 #18441.

@Janpot
Copy link
Member Author

Janpot commented Dec 10, 2019

Yes, I saw, but it also happens in latest version of next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants