-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Added snapshots tests for grids #4559
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
Added snapshot testing for the new grid layout feature - In `snapshots`, added the new `gridTests.tsx` file that supports testing grids in the themes - Also cleaned up the other tests little bit - In `core` added `GridSnap.tsx` and changed the other snapshot tests to be `tsx` files - Also updated `LayoutGridField` to add support for the `$lookup=` prefix on string props in the children field of a gridSchema - Updated the `LayoutGridField` tests accordingly - Added the new snapshot files (and deleted the old .jsx` ones) - In `antd`, `chakra`, `fluentui-rc`, `mui`, `react-bootstrap`, `semantic-ui` and `shadcdn` added `GridSnap.tsx` - Added the new snapshot files after running the test - In playground, updated the `layoutGrid.tsx` to add a `formContext` for the `PlaceholderText` lookup
// @ts-expect-error TS2740 because it is missing all of the FieldProps, which we don't need | ||
rendered: <TestRenderer data-testid={LayoutGridField.TEST_IDS.uiComponent} fullWidth />, |
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.
I'm surprised this equality check works, I've never thought to compare two React elements. Does this assume React.createElement
is deterministic and always returns objects that can be deeply compared? Is that a safe assumption?
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.
Good question... I just know it works, at least with Jest deep equality :)
packages/mui/test/GridSnap.test.tsx
Outdated
jest.spyOn(window, 'getComputedStyle').mockImplementation(() => { | ||
return { | ||
width: 100, | ||
'box-sizing': 10, | ||
'padding-bottom': 1, | ||
'padding-top': 1, | ||
'border-bottom-width': 1, | ||
'border-top-width': 1, | ||
} as unknown as CSSStyleDeclaration; | ||
}); |
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.
is this mock implementation copied from another test suite? would it make sense to put it all in one place?
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.
Refactored
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.
Couple of questions, but overall LGTM. Thanks for the comprehensive approach for each theme
Reasons for making this change
Added snapshot testing for the new grid layout feature
snapshots
, added the newgridTests.tsx
file that supports testing grids in the themescore
addedGridSnap.tsx
and changed the other snapshot tests to betsx
filesLayoutGridField
to add support for the$lookup=
prefix on string props in the children field of a gridSchemaLayoutGridField
tests accordinglyantd
,chakra
,fluentui-rc
,mui
,react-bootstrap
,semantic-ui
andshadcdn
addedGridSnap.tsx
layoutGrid.tsx
to add aformContext
for thePlaceholderText
lookupChecklist
npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update
to update snapshots, if needed.