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: Filter mount stories from Stories block, error when referenced in MDX #28434

Merged
merged 11 commits into from
Jul 3, 2024

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Jul 3, 2024

Closes #28420

What I did

Filter mount stories inside of Stories and give a proper error message when used in MDX

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

  • Make sure this PR contains one of the labels below:

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

Greptile Summary

  • Filtered mount stories within /code/lib/blocks/src/blocks/Stories.tsx
  • Added error handling for mount stories referenced in MDX in /code/core/src/preview-errors.ts
  • Updated Babel preset in /code/frameworks/nextjs/src/babel/preset.ts for specific browser targets and bugfixes
  • Added CommonJS export in /code/lib/cli/core/components/index.cjs
  • Deleted obsolete Storybook stories in /code/core/template/stories/

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • code/core/src/preview-api/modules/preview-web/Preview.tsx: Replaced mountDestructured with direct check for r.story.usesMount.
  • code/core/src/preview-api/modules/preview-web/render/StoryRender.ts: Simplified logic by directly checking story.usesMount.
  • code/core/src/preview-api/modules/store/csf/portable-stories.ts: Removed mountDestructured import, modified isMountDestructured logic.
  • code/core/src/preview-errors.ts: Added PlayFunctionWithMountNotAvailableError class under BLOCKS category.
  • code/lib/blocks/src/blocks/Stories.tsx: Filtered out stories using 'mount' in docs to prevent rendering issues.

9 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

code/core/src/preview-errors.ts Outdated Show resolved Hide resolved
Copy link

nx-cloud bot commented Jul 3, 2024

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • code/core/src/preview-api/modules/store/csf/prepareStory.ts: Replaced generic error with NoRenderFunctionError for missing render function.
  • code/core/src/preview-errors.ts: Introduced NoRenderFunctionError for better error handling when no render function is available.

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

code/core/src/preview-errors.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Removed PlayFunctionWithMountNotAvailableError class in code/core/src/preview-errors.ts
  • Updated error message in NoRenderFunctionError class in code/core/src/preview-errors.ts
  • Simplified error handling by consolidating rendering function errors
  • Corrected typo in NoRenderFunctionError class message

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@kasperpeulen kasperpeulen requested a review from yannbf July 3, 2024 14:00
# Conflicts:
#	code/core/src/preview-api/modules/preview-web/render/StoryRender.ts
#	code/core/src/preview-errors.ts
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Modified PreviewWithSelection.tsx to streamline story rendering by passing renderToCanvas directly and adding showStoryDuringRender callback.
  • Updated StoryRender.test.ts with beforeEach hook, introduced mountSpy function, and enhanced error handling tests.
  • Enhanced StoryRender.ts with NoStoryMountedError, improved runPhase method, and refined mount function for better error scenarios.
  • Added NoStoryMountedError in preview-errors.ts for clearer error messages in MDX scenarios.

4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added usesMount property to story objects in /code/core/src/preview-api/modules/store/StoryStore.test.ts
  • Ensured usesMount is set to false in various test scenarios in StoryStore.test.ts
  • Added usesMount field to prepared story object in /code/core/src/preview-api/modules/store/csf/prepareStory.test.ts
  • Verified usesMount is carried through from story annotations in prepareStory.test.ts

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@kasperpeulen kasperpeulen changed the base branch from next to kasper/phases July 3, 2024 15:15
@kasperpeulen kasperpeulen changed the base branch from kasper/phases to next July 3, 2024 15:16
@shilman shilman changed the title Docs: Filter mount stories inside of Stories and give a proper error message when used in MDX Docs: Filter mount stories in Stories, give error message when used in MDX Jul 3, 2024
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.

Minor change on PR cleanliness -- not sure whether to address this here or in #28431

return (
<ErrorMessage>
This story mounts inside of play. Set{' '}
<a href="https://storybook.js.org/docs/api/doc-blocks/doc-block-story#autoplay">autoplay</a>{' '}
Copy link
Member

Choose a reason for hiding this comment

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

Make sure to track the docs update as a followup issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

As an alternative, we might just not filter these stores in Stories and instead use this message? Would that solve the discoverability issue?

The user could just set tags: ['-autoplay'] on the story to avoid the error, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could live with that, although the error is quite a bit ugly atm, cc @shilman

stories = stories.filter((story) => story.tags?.includes('autodocs'));
// Don't show stories where mount is used in docs.
// As the play function is not running in docs, and when mount is used, the mounting is happening in play itself.
stories = stories.filter((story) => story.tags?.includes('autodocs') && !story.usesMount);
Copy link
Member

Choose a reason for hiding this comment

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

Probably also want to document this somewhere, either in this PR or in a tracked followup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tracked here: #28442

}

if (!mounted) {
throw new NoStoryMountedError();
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate with #28431 .. were these PRs up split properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry changed the base branch now

@shilman shilman changed the title Docs: Filter mount stories in Stories, give error message when used in MDX Docs: Handle mount stories - filter from Stories block, error when referenced in MDX Jul 3, 2024
@shilman shilman changed the title Docs: Handle mount stories - filter from Stories block, error when referenced in MDX Docs: Filter mount stories from Stories block, error when referenced in MDX Jul 3, 2024
@kasperpeulen kasperpeulen changed the base branch from next to kasper/phases July 3, 2024 16:46
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

# Conflicts:
#	code/core/src/preview-api/modules/preview-web/Preview.tsx
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

30 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Base automatically changed from kasper/phases to next July 3, 2024 18:58
@kasperpeulen kasperpeulen changed the base branch from next to kasper/next-babel-preset July 3, 2024 20:03
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

9 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

LGTM

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Base automatically changed from kasper/next-babel-preset to next July 3, 2024 21:42
@kasperpeulen kasperpeulen merged commit fc64e22 into next Jul 3, 2024
54 checks passed
@kasperpeulen kasperpeulen deleted the kasper/docs-filtering branch July 3, 2024 22:24
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.

Move docs filter logic of mount stories to the Stories block
3 participants