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

Core: Fix ability to use component-level play functions #17817

Merged
merged 5 commits into from
Nov 30, 2022

Conversation

wKich
Copy link
Member

@wKich wKich commented Mar 29, 2022

Issue: Nope

What I did

Fix ability to use play function defined in Meta level

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.

@nx-cloud
Copy link

nx-cloud bot commented Mar 29, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d5cd800. 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


🟥 Failed Commands
nx run-many --target=prepare --all --parallel --max-parallel=15 -- --optimized

Sent with 💌 from NxCloud.

@wKich wKich requested a review from yannbf March 29, 2022 10:29
@wKich wKich changed the title Fix ability to use play function defined in Meta or Project levels Fix ability to use play function defined in Meta level Mar 29, 2022
@shilman shilman changed the title Fix ability to use play function defined in Meta level Core: Fix ability to use component-level play functions Mar 30, 2022
@shilman
Copy link
Member

shilman commented Mar 30, 2022

@wKich getting a typescript error in CI. can you please update the types?

ERR! FAILED (ts) : src/csf/prepareStory.ts:198:51 - error TS2339: Property 'play' does not exist on type 'NormalizedComponentAnnotations<TFramework>'.

198     storyAnnotations.play || componentAnnotations.play;

@wKich
Copy link
Member Author

wKich commented Mar 30, 2022

Hm... It looks like @storybook/csf was changed

@wKich
Copy link
Member Author

wKich commented Mar 30, 2022

@shilman Should I revert that commit ComponentDriven/csf@85efd3c ?

@ndelangen
Copy link
Member

@shilman @yannbf could you put this on your todo-list to review and or respond? 👏👏👏

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.

LGTM. I think it was @tmeasday who might've had objections to the component-level play function

@tmeasday
Copy link
Member

tmeasday commented Jul 7, 2022

I don't have an issue unless I am forgetting something.

@yannbf
Copy link
Member

yannbf commented Jul 13, 2022

@shilman let's proceed with this change! But first merge ComponentDriven/csf#40 and release a new version of course

@yannbf yannbf added the linear label Nov 21, 2022
@socket-security
Copy link

socket-security bot commented Nov 28, 2022

Socket Security Pull Request Report

👍 No dependency changes detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

Powered by socket.dev

@@ -3,16 +3,16 @@
"version": "7.0.0-alpha.16",
"private": true,
"scripts": {
"docs:prettier:check": "cd ../docs && prettier --check ./snippets",
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this change happened in the PR but I guess it makes sense, with the scripts being ordered alphabetically

@yannbf
Copy link
Member

yannbf commented Nov 30, 2022

Merging this as the failure is unrelated and has been fixed in next!

@yannbf yannbf merged commit 1b47166 into next Nov 30, 2022
@yannbf yannbf deleted the wkich/play-function-override branch November 30, 2022 18:25
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.

5 participants