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.

Sign up for GitHub

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

Jump to bottom

fix broken mdx links #17080

Closed
wants to merge 2 commits into from
Closed

fix broken mdx links #17080

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc8a98b
fix broken mdx links
Dec 27, 2021
629f503
Merge branch 'storybookjs:next' into mdx-links
sag1v Dec 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion addons/docs/src/blocks/mdx.tsx
View file
Open in desktop
Expand Up @@ -49,8 +49,16 @@ export const CodeOrSourceMdx: FC<CodeOrSourceMdxProps> = ({ className, children,
);
};

function generateHrefWithHash(hash: string): string {
// eslint-disable-next-line no-undef
const location = window.parent ? window.parent.location : window.location;
Copy link
Member

Choose a reason for hiding this comment

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

This is an illegal operation cross origin!

Copy link
Member

Choose a reason for hiding this comment

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

which can happen when a storybook is composed in:
https://storybook.js.org/docs/react/sharing/storybook-composition#gatsby-focus-wrapper

Copy link
Author

Choose a reason for hiding this comment

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

@ndelangen Got any suggestions to update the location?

Copy link
Author

Choose a reason for hiding this comment

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

BTW, your link seems to point to a header called "gatsby-focus-wrapper" but i don't see it in the page

Copy link
Member

Choose a reason for hiding this comment

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

maybe the check should be Element instead of HTMLElement WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I think that the issue is not the check but the fact that we're not passing the correct URL
We are passing only the hash and not appending it to the already existing URL or replacing it with existing hash in the URL.

Copy link
Author

@sag1v sag1v Jan 5, 2022 •
edited

Choose a reason for hiding this comment

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

When we call addons.getChannel().emit(NAVIGATE_URL, url), where is the actual code that navigating? Maybe we will have access to the full URL there and we can change the logic there.

There's a getBase function in router.tsx

const getBase = () => `${document.location.pathname}?`;

What are the implications of document.location vs window.location?

BTW, the links in the side navigation are working perfectly, maybe we can re-use their logic?

const href = `${location.search}${hash}`;
return href;
}

function navigate(url: string) {
addons.getChannel().emit(NAVIGATE_URL, url);
const urlWithHash = generateHrefWithHash(url);
addons.getChannel().emit(NAVIGATE_URL, urlWithHash);
}

// @ts-ignore
Expand Down