-
-
Notifications
You must be signed in to change notification settings - Fork 189
test(storybook): ActionIcon click DEV-98 #5669
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
Conversation
# Conflicts: # jsapp/js/components/common/actionIcon.stories.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
jsapp/js/components/common/actionIcon.stories.tsx:97
- [nitpick] For a more robust test, consider asserting the exact number of times the onClick handler is expected to be called (e.g., using toHaveBeenCalledTimes(1)) rather than simply checking if it was called.
await expect((args as StoryArgs).onClick).toHaveBeenCalled()
Co-authored-by: Akuukis <Akuukis@users.noreply.github.com>
Co-authored-by: Akuukis <Akuukis@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
jsapp/js/components/common/actionIcon.stories.tsx:90
- [nitpick] Consider using 'data-testid' instead of 'id' for test elements to align with the StoryArgs type and common testing conventions.
id: 'ActionIcon-click-test',
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
jsapp/js/components/common/actionIcon.stories.tsx:99
- [nitpick] Consider refining the Story type definitions to avoid the need for casting args. This can help maintain strong type safety throughout your tests.
await expect((args as StoryArgs).onClick).toHaveBeenCalledTimes(1)
jsapp/js/components/common/ActionIcon.tsx:22
- [nitpick] Setting displayName on the component before wrapping it with createPolymorphicComponent may not propagate as expected. Verify that the intended displayName appears correctly in React DevTools.
ActionIcon.displayName = 'ActionIcon'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested, one nitpick
// TODO: I made an issue to point this out to Storybook team: https://github.com/storybookjs/storybook/issues/31106 | ||
// let's fix this when they fix it. | ||
const canvas = within(canvasElement) | ||
await userEvent.click(canvas.getByTestId(testId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks more scalable to me in case of more than single test case.
await userEvent.click(canvas.getByTestId(testId)) | |
await userEvent.click(canvas.getByTestId((args as StoryArgs)['data-testid']!)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure :)
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global💭 Notes
I was trying to come up with some "proper" way of having types in
args
, but searching Storybook docs and thier GH gave me nothing useful. I made an issue to kindly point this out to Storybook team: storybookjs/storybook#31106.Other than that, it's just a simple test to show how it's done ;)
👀 Preview steps
Testing: