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

Make ID of root component configurable #11296

Closed
schicks opened this issue Jun 24, 2020 · 15 comments
Closed

Make ID of root component configurable #11296

schicks opened this issue Jun 24, 2020 · 15 comments

Comments

@schicks
Copy link

schicks commented Jun 24, 2020

Is your feature request related to a problem? Please describe.
I have an application that always lives within a third party frame that provides some context that the components rely on. This frame is outside my control, and assumes that there will be some root level structure on the page. It also assumes that all content will be rendered within a <main id="ps-main"> tag.

Describe the solution you'd like
A way to provide to storybook the id of the element to use as the react root when a preview-body is provided.

Describe alternatives you've considered
Without a way to configure what the react root that storybook uses is, the only workaround I have is to supply a preview-body with incomplete html, omitting the closing tag of the main so that storybook will render content within it. Not only does this feel gross, it also still inserts additional elements between my app and the surrounding frame, which occasionally causes problems.

Are you able to assist bring the feature to reality?
Yes, I can... I haven't worked on storybook before but I'd be happy to submit a PR if this sounds like something that could get merged.

@shilman
Copy link
Member

shilman commented Jun 25, 2020

Related: #10638

@schicks
Copy link
Author

schicks commented Jun 25, 2020

@shilman, I don't have a lot of context on that pr but it doesn't look too me like it wouldn't solve my problem, because it still requires a particular ID for the root element. I would like to be able to set the root ID.

@shilman
Copy link
Member

shilman commented Jun 25, 2020

Cc @ndelangen

@schicks
Copy link
Author

schicks commented Jun 25, 2020

Just read through the linked issue thread. It seems to me that making the root ID configurable but leaving #root as the default would solve my problem and those problems, and also avoid a breaking change / headaches for existing integrations.

@shilman
Copy link
Member

shilman commented Jun 25, 2020

Can you use a decorator to set up the context? Possibly in combination with preview-head.html?

@schicks
Copy link
Author

schicks commented Jun 25, 2020

I can try a decorator, though it isn't my ideal solution. I reached for preview body because I wanted to be able to have the root of the document be built there, which (as the other issue mentioned) is implied in the current documentation. If I can't change the root then I don't think preview body helps me. I'll get back to you after I try the decorator though.

@schicks
Copy link
Author

schicks commented Jun 25, 2020

The decorator approach didn't work. I'm not completely sure, but my suspicion is that the iframe gets reused between stories, and the framing component is not really designed to be unloaded and reloaded repeatedly. This wouldn't be a problem with the preview-body approach (with a custom root) because it appears from the incomplete dom solution that I initially posted that the preview-body content is also persisted between stories.

For the decorator approach to work there would need to be some guarantee that either A) all window state would be forgotten between stories and all DOM above the storybook root would be reset between stories or B) the dom created by the decorator was persisted between stories and not interfered with. Neither of these sound like a good idea to me; allowing a configurable custom root seems much simpler.

@shilman
Copy link
Member

shilman commented Jun 26, 2020

@schicks Ok, I'd like to see who else has this problem--this is the first time we've received this request after tens of thousands of projects using Storybook. Adding a configurable root is easy from a code standpoint, but would be a maintenance nightmare, so I don't think it's going to happen.

@schicks
Copy link
Author

schicks commented Jun 26, 2020

That's too bad. What makes it so much harder to maintain than the method in #10638?

@shilman
Copy link
Member

shilman commented Jun 27, 2020

@schicks every time you make something configurable, you have to read the configuration value in every place that it's used, including third party tools (of which there are many) that rely on the page structure. Having a fixed page structure avoids that.

@schicks
Copy link
Author

schicks commented Jun 27, 2020

Alright. It does seem worth changing these docs then. At the moment they are somewhat misleading.

@shilman
Copy link
Member

shilman commented Jun 27, 2020

@schicks good call on the documentation -- WDYT about the fix in #11337 ?

@ndelangen
Copy link
Member

Yeah I'm pretty sure we're not going to make this configurable, in fact the rename got rescheduled for 7.0.0, because we don't want to disturb the ecosystem too much.

@stale
Copy link

stale bot commented Jul 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jul 25, 2020
@stale
Copy link

stale bot commented Aug 24, 2020

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants