-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: Testing Module UI improvements #30773
base: next
Are you sure you want to change the base?
Conversation
View your CI Pipeline Execution ↗ for commit 82bd173.
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
…lay to hover tooltips
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.
22 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
(await canvas.findByLabelText('Enable watch mode')).click(); | ||
await expect(mockStore.setState).toHaveBeenCalledOnce(); | ||
|
||
await expect(await canvas.findByLabelText('Coverage (unavailable)')).not.toBeDisabled(); |
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.
logic: This assertion may be flaky since it checks for a disabled state that depends on watch mode. Consider adding a wait or checking the state transition.
export const AccessibilityViolations: Story = { | ||
args: Default.args, | ||
beforeEach: async () => { | ||
mockStore.setState({ | ||
...storeOptions.initialState, | ||
config: { ...storeOptions.initialState.config, a11y: true }, | ||
}); | ||
}, | ||
play: async ({ canvas }) => { | ||
userEvent.hover(await canvas.findByLabelText(/Accessibility status:/)); | ||
}, |
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.
logic: AccessibilityViolations story doesn't actually simulate any violations - it's identical to AccessibilityEnabled. Should include mock violation data to properly test this state.
state.running && config.coverage | ||
? 'Testing in progress' |
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.
logic: Using config.coverage here instead of config.a11y for accessibility testing tooltip
state.running && config.coverage | |
? 'Testing in progress' | |
state.running && config.a11y | |
? 'Testing in progress' |
clearStatuses: () => { | ||
// TODO | ||
}, |
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.
logic: Empty placeholder function needs implementation for clearing statuses. This is a critical missing piece that should be implemented before merging.
@@ -166,6 +166,10 @@ export const SidebarBottomBase = ({ | |||
<TestingModule | |||
{...{ | |||
testProviders: testProvidersArray, | |||
statusCount: Object.keys(status).length, |
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.
style: Using Object.keys(status).length may not accurately reflect the actual number of statuses since status is a nested object structure. Consider using a more precise counting method.
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.
yes, this would only count the number of stories with statuses, not the total number of statuses.
the way it is used below, you could probably just make it a hasStatuses
-boolean? When my PR is merged, we could also perhaps get the data directly in TestingModule
with the useStatus
hook and a selector
@@ -5,23 +5,23 @@ import type { TestProviders } from 'storybook/internal/core-events'; | |||
import { useStorybookApi } from 'storybook/internal/manager-api'; |
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.
Do you agree that we should get rid of this LegacyRender for 9.0, and migrate VTA to the modern one?
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.
When we update VTA to use UniversalStore, it's going to have to use a Storybook version that supports it. Once we do that, we can update VTA to provide its own rendering and get rid of LegacyRender. The reason LegacyRender is still around is because older VTA versions will still be using it. We need to make a clean cut at some point, where a certain VTA version needs a certain SB version and vice-versa.
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.
Code looks great! ✨ Reviewing functionality and stories tomorrow.
Closes #30746
What I did
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/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 pull request has been released as version
0.0.0-pr-30773-sha-82bd1734
. Try it out in a new sandbox by runningnpx [email protected] sandbox
or in an existing project withnpx [email protected] upgrade
.More information
0.0.0-pr-30773-sha-82bd1734
test-polish
82bd1734
1741797433
)To request a new release of this pull request, mention the
@storybookjs/core
team.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=30773
Greptile Summary
This PR enhances the Testing Module UI with significant usability improvements, focusing on more direct interactions and clearer status indicators.
code/addons/test/src/components/TestProviderRender.tsx
in favor of direct option updatesopenTestsPanel
andopenA11yPanel
functions incode/core/src/manager/components/sidebar/TestingModule.tsx
code/core/src/manager/components/sidebar/SidebarBottom.tsx
code/core/src/components/components/tooltip/WithTooltip.tsx