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

Addon-docs: Support absolute anchors when deployed at non-root #11403

Merged
merged 2 commits into from
Jul 30, 2020

Conversation

amannn
Copy link
Contributor

@amannn amannn commented Jul 3, 2020

Issue: #11404

What I did

When Storybook is deployed at an URL like https://example.com/storybook/component-library/v48.3.2/, the anchors within docs content will remove the base url.

E.g. when you're on https://example.com/storybook/component-library/v48.3.2/?path=/docs/components-icon--default and you have a link to /docs/components-button--default in the documentation, the resulting link will point to https://example.com/?path=/docs/components-button--default.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

Not sure if there's a good way to add a test for this, as it depends on a special kind of deployment?

// If storybook is hosted at a non-root path (e.g. `/storybook/`),
// the base url needs to be prefixed to storybook paths.
let storybookBaseUrl =
typeof window !== 'undefined' ? window.parent.document.location.pathname : '/';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This access should be safe, even if the content is rendered outside of an iframe.

From MDN:

If a window does not have a parent, its parent property is a reference to itself.

@amannn
Copy link
Contributor Author

amannn commented Jul 3, 2020

I'm not sure why some tests failed. Are they related to my change? Can you help me with them?

@amannn amannn marked this pull request as ready for review July 3, 2020 09:45
@amannn amannn requested review from ndelangen and tmeasday as code owners July 3, 2020 09:45
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Nice one @amannn!

Were you able to test it on your dev machine somehow? How about:

yarn bootstrap --core
cd examples/official-storybook
yarn build-storybook
npx http-server
open localhost:9011/storybook-static

@amannn
Copy link
Contributor Author

amannn commented Jul 6, 2020

@shilman Thanks for the review and your snippet!

I've tested it previously in a project of mine by editing node_modules 😬 , but I've just confirmed that this also works in this repo.

I've added:

 * [Go to color palette](/docs/basics-colorpalette--page)

to lib/components/src/blocks/Description.tsx:14 and opened Storybook with:

open "http://localhost:8080/storybook-static/?path=/docs/docs-description--text"

(note that http-server uses port 8080 by default).

The anchor is rendered correctly and the link takes you to the color palette 🙂 .

I wasn't sure if there's any reasonable place in the code base where such an anchor would make sense, so we have a story to test this behaviour?

@shilman shilman changed the title Support absolute anchors when Storybook is deployed at non-root Core: Support absolute anchors when deployed at non-root Jul 8, 2020
@shilman shilman changed the title Core: Support absolute anchors when deployed at non-root Addon-docs: Support absolute anchors when deployed at non-root Jul 8, 2020
@shilman shilman added this to the 6.0 docs milestone Jul 8, 2020
@amannn
Copy link
Contributor Author

amannn commented Jul 28, 2020

@shilman is there anything I can do to get this merged?

@shilman shilman merged commit f2b497b into storybookjs:next Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants