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: Fix React Profiler in source snippets #19004

Merged

Conversation

zhyd1997
Copy link
Contributor

@zhyd1997 zhyd1997 commented Aug 24, 2022

Issue: #11554

What I did

  • enabled code/renderers unit tests.
  • React Profiler support.
  • added stories.

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.

@zhyd1997 zhyd1997 marked this pull request as ready for review August 24, 2022 17:41
@zhyd1997 zhyd1997 changed the title fix: supported React.Profiler use in component. fix: supported React Profiler use in component. Aug 24, 2022
@@ -101,6 +101,7 @@ export const renderJsx = (code: React.ReactElement, options: JSXOptions) => {
// To get exotic component names resolving properly
displayName: (el: any): string =>
el.type.displayName ||
(el.type === Symbol.for('react.profiler') ? 'Profiler' : null) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the major change here!

@@ -141,7 +142,7 @@ export const renderJsx = (code: React.ReactElement, options: JSXOptions) => {
return string;
}).join('\n');

return result.replace(/function\s+noRefCheck\(\)\s+\{\}/, '() => {}');
return result.replace(/function\s+noRefCheck\(\)\s+\{\}/g, '() => {}');
Copy link
Contributor Author

@zhyd1997 zhyd1997 Aug 25, 2022

Choose a reason for hiding this comment

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

const Template: ComponentStory<typeof Button> = (args) => 
  <Profiler id="test" onRender={() => console.log('test-profiler')}>
    <Button {...args} />
  </Profiler>

export const Primary = Template.bind({});
// More on args: https://storybook.js.org/docs/react/writing-stories/args
Primary.args = {
  primary: true,
  label: 'Button',
  onClick: () => console.log('test-button')
};

Before:
Screen Shot 2022-08-25 at 12 47 42

After:
Screen Shot 2022-08-25 at 12 46 58

Copy link

@ns-kutsav ns-kutsav Oct 14, 2023

Choose a reason for hiding this comment

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

It still isn't showing the actual function. Is that the expected behavior?

Copy link
Contributor

@kylegach kylegach left a comment

Choose a reason for hiding this comment

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

Hey, @zhyd1997! This is an amazing first-time contribution! 🥳

Would you mind checking to see if the change related to noRefCheck is tested, and adding a test, if not. Thanks!

@@ -87,7 +87,7 @@ describe('Extracting Arguments', () => {
"control": Object {
"type": "boolean",
},
"description": null,
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhyd1997 — Can you please share why this change was necessary?

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 didn't change svelte code, and I think they 're all falsy value, so I just ran something like update-snapshots command.

Copy link
Contributor Author

@zhyd1997 zhyd1997 Aug 29, 2022

Choose a reason for hiding this comment

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

@kylegach

Previously we didn't enable code/renderers unit test after moved all source code into code folder, but I need run unit test for react, so I manually enabled it in this PR, but svelte unit test failed, that's why I updated all code/renders snapshots here.

@zhyd1997
Copy link
Contributor Author

zhyd1997 commented Aug 29, 2022

Hey, @zhyd1997! This is an amazing first-time contribution! 🥳

Would you mind checking to see if the change related to noRefCheck is tested, and adding a test, if not. Thanks!

I need to add a story to test it, but don't know which example under examples folder should I take cra-ts-kitchen-sink or react-ts?

@zhyd1997 zhyd1997 requested a review from kylegach September 1, 2022 02:52
@iosifnicolae2
Copy link

Any news on this issue? It would be awesome if we can gave good code export functionality.

Thank you!

@shilman shilman changed the title fix: supported React Profiler use in component. Addon-docs: Fix React Profiler in source snippets Oct 27, 2022
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.

Thank you @zhyd1997 !!! 🙌

@shilman shilman merged commit 8009143 into storybookjs:next Oct 27, 2022
@zhyd1997 zhyd1997 deleted the fix/support-profiler-use-in-component branch October 27, 2022 22:16
@365kim
Copy link

365kim commented Jan 29, 2023

Is this fix available in any version of @storybook/react? 🙏

@zhyd1997
Copy link
Contributor Author

Is this fix available in any version of @storybook/react? 🙏

Sorry no, available from this version: v7.0.0-alpha.46

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.

6 participants