-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore(react-infobutton): Adding vr tests for InfoButton and updating conformance tests #25682
Conversation
📊 Bundle size reportUnchanged fixtures
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0fefe44:
|
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: c7a1e243f8366456cb12af369ae5b749aabaec95 (build) |
|
||
const infoButtonContent = 'This is the content of an InfoButton.'; | ||
|
||
storiesOf('InfoButton', module) |
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.
It would be good to add tests for the hover, pressed, and keyboard focused states. (As a rough guideline, VR tests should cover all of the states in useInfoButtonStyles.)
Also, it should crop to a test wrapper if possible, to reduce the size of the image.
storiesOf('InfoButton', module) | |
storiesOf('InfoButton', module) | |
.addDecorator(TestWrapperDecoratorFixedWidth) | |
.addDecorator(story => ( | |
<Screener | |
steps={new Steps() | |
.snapshot('rest', { cropTo: '.testWrapper' }) | |
.hover('button') | |
.snapshot('hover', { cropTo: '.testWrapper' }) | |
.focus('button') | |
.snapshot('focus', { cropTo: '.testWrapper' }) | |
.mouseDown('button') | |
.snapshot('active', { cropTo: '.testWrapper' }) | |
.end()} | |
> | |
{story()} | |
</Screener> | |
)) |
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.
You might need to split out the size stories into a separate call to storiesOf
, so that the state tests only have a single InfoButton in them.
id="show-content-small" | ||
size="small" | ||
content={{ children: infoButtonContent }} | ||
popover={{ open: true } as PopoverProps} |
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.
It would be good to fix whatever is making it so you need this as PopoverProps
cast. Ideally it should not be necessary here. This can be fixed in a separate PR though, since it's not related to this change.
apps/vr-tests-react-components/src/stories/InfoButton.stories.tsx
Outdated
Show resolved
Hide resolved
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 1581 | 1570 | 5000 | |
Button | mount | 1161 | 1125 | 5000 | |
FluentProvider | mount | 1946 | 1952 | 5000 | |
FluentProviderWithTheme | mount | 763 | 784 | 10 | |
FluentProviderWithTheme | virtual-rerender | 716 | 695 | 10 | |
FluentProviderWithTheme | virtual-rerender-with-unmount | 746 | 754 | 10 | |
MakeStyles | mount | 2251 | 2294 | 50000 | |
SpinButton | mount | 3018 | 3112 | 5000 |
.addStory( | ||
'size-open', | ||
() => ( | ||
<div style={{ display: 'flex', flexDirection: 'column', padding: '60px', gap: '80px', alignItems: 'start' }}> |
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.
It looks like this padding is causing the balloon to go off the edge of the TestWrapper. Maybe change it only to top padding?
<div style={{ display: 'flex', flexDirection: 'column', padding: '60px', gap: '80px', alignItems: 'start' }}> | |
<div style={{ display: 'flex', flexDirection: 'column', paddingTop: '60px', gap: '80px', alignItems: 'start' }}> |
</Screener> | ||
)) | ||
.addStory( | ||
'size', |
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.
'size', | |
'default', |
.addStory( | ||
'size', | ||
() => ( | ||
<div style={{ display: 'flex', flexDirection: 'column', padding: '60px', gap: '80px', alignItems: 'start' }}> | ||
<InfoButton size="small" content="This is the content of an InfoButton." /> | ||
<InfoButton size="medium" content="This is the content of an InfoButton." /> | ||
<InfoButton size="large" content="This is the content of an InfoButton." /> | ||
</div> | ||
), | ||
{ | ||
includeHighContrast: true, | ||
includeDarkMode: true, | ||
}, | ||
) |
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.
You can probably get away with only size-open
, since the rest of the visuals are tested by the default case.
.addStory( | |
'size', | |
() => ( | |
<div style={{ display: 'flex', flexDirection: 'column', padding: '60px', gap: '80px', alignItems: 'start' }}> | |
<InfoButton size="small" content="This is the content of an InfoButton." /> | |
<InfoButton size="medium" content="This is the content of an InfoButton." /> | |
<InfoButton size="large" content="This is the content of an InfoButton." /> | |
</div> | |
), | |
{ | |
includeHighContrast: true, | |
includeDarkMode: true, | |
}, | |
) |
}, | ||
); | ||
|
||
storiesOf('InfoButton - sizes', module) |
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.
Keep this name the same so they're grouped together:
storiesOf('InfoButton - sizes', module) | |
storiesOf('InfoButton', module) |
storiesOf('InfoButton - sizes', module) | ||
.addDecorator(TestWrapperDecoratorFixedWidth) | ||
.addDecorator(story => ( | ||
<Screener steps={new Screener.Steps().snapshot('active', { cropTo: '.testWrapper' }).end()}>{story()}</Screener> |
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.
<Screener steps={new Screener.Steps().snapshot('active', { cropTo: '.testWrapper' }).end()}>{story()}</Screener> | |
<Screener steps={new Screener.Steps().snapshot('rest', { cropTo: '.testWrapper' }).end()}>{story()}</Screener> |
.focus('#info-button') | ||
.snapshot('focus', { cropTo: '.testWrapper' }) |
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 PR adds the following:
Related Issue(s)
Fixes #25620