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

Future - Rename Storybook DOM root IDs #10638

Merged
merged 6 commits into from
Aug 23, 2022
Merged

Future - Rename Storybook DOM root IDs #10638

merged 6 commits into from
Aug 23, 2022

Conversation

ndelangen
Copy link
Member

Issue: #10403

What I did

I changed #root => #storybook-root
I changed #docs-root => #storybook-docs

@ndelangen ndelangen self-assigned this May 4, 2020
@ndelangen ndelangen added BREAKING CHANGE maintenance User-facing maintenance tasks labels May 4, 2020
@ndelangen ndelangen added this to the 6.0 milestone May 4, 2020
@shilman shilman changed the title Tech/root change Core: Rename Storybook DOM root IDs May 7, 2020
@stale stale bot added the inactive label Jun 10, 2020
@storybookjs storybookjs deleted a comment from stale bot Jun 10, 2020
@stale stale bot removed the inactive label Jun 10, 2020
@ndelangen
Copy link
Member Author

Do we want to put this in v6.0.0?

If we're not doing this now, it'll be 7.0.0 at the earliest.

@shilman shilman modified the milestones: 6.0, 6.0 breaking Jun 19, 2020
@ndelangen
Copy link
Member Author

@tmeasday @shilman WDYT, should we push this forward for 6.0.0 or delay for 7.0.0?

@shilman shilman removed this from the 6.0 breaking milestone Jun 30, 2020
@ndelangen ndelangen added this to the 7.0 milestone Jul 1, 2020
@stale stale bot added the inactive label Jul 25, 2020
@stale stale bot removed the inactive label Jul 27, 2020
@ndelangen ndelangen changed the base branch from next to future/base June 23, 2022 18:34
@nx-cloud
Copy link

nx-cloud bot commented Jun 23, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4a605a4. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen
Copy link
Member Author

What pain do you think users or us storybook maintainers would endure if we pushed this into 7.0?

@ndelangen
Copy link
Member Author

So the possible foreseeable pain for users is re seeing to portals that target #root.
That's an anti-pattern anyway, because that can't work for docs.

So I think breaking that is the right thing to do.

Another point is that we don't document what div target storybook renders to at all. The fact we target #root is an implementation detail that's not supposed to be leveraged by users.

I'd expect users to render portals to either document.body OR to use the canvasElement provided in docs OR wrap the story in a decorator setting up a selector to render to.

I think there should be a task to make targeting the current story container real easy for users.

@ndelangen
Copy link
Member Author

So we need 2 things to get this merged/ready for release:

  • chromatic needs to be updated, otherwise we lose a very significant part of our testing strategy. @skitterm is this something you could discuss with the apps team?
  • we need a change to the migration document @jonniebigodes is this something you could put on your task list?

Additionally we should have a task to create a real api (on the storyContext?) to get the selector for the container of the current story. @shilman that is probably something the CSF team (possibly myself?) will have to do. Let's chat about that soon.

@tmeasday
Copy link
Member

tmeasday commented Jul 4, 2022

@ndelangen - the StoryContext already has CanvasElement which I think is what you want here: https://github.com/ComponentDriven/csf/blob/4566f4da20ea41a599548128cf993d1bae7d1d46/src/story.ts#L106

Also @jmhobbs or @codykaup would be the ones to talk about changing Chromatic's capture to handle a different #root element.

Base automatically changed from future/base to next July 25, 2022 10:37
I changed #docs-root => #storybook-docs
@ndelangen
Copy link
Member Author

@tmeasday @shilman @yannbf shall I merge this?

@ndelangen ndelangen requested review from yannbf and removed request for kazupon, alterx, thomasbertet, jbovenschen, igor-dv and CodeByAlex August 2, 2022 09:55
@ndelangen
Copy link
Member Author

@tmeasday could I get a review on this?

@ndelangen
Copy link
Member Author

@yannbf we need a minor change in the test-runner so it checks both the IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants