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.

Sign up for GitHub

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

Jump to bottom

Components: Fix no-node-access in Grid tests #45900

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Components: Fix no-node-access in Grid tests #45900

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b500ef3
Components: Fix no-node-access in Grid tests
tyxla Nov 18, 2022
f47bd07
role -> data-testid
tyxla Nov 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 31 additions & 31 deletions packages/components/src/grid/test/grid.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { render } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

/**
* Internal dependencies
Expand All @@ -12,152 +12,152 @@ import CONFIG from '../../utils/config-values';

describe( 'props', () => {
test( 'should render correctly', () => {
const { container } = render(
<Grid>
render(
<Grid role="grid">
Copy link
Member Author

Choose a reason for hiding this comment

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

I was contemplating whether it made sense to add the grid role by default to <Grid /> and then the gridcell role to each child view, but then I realized it probably won't always be the case that this makes sense. So it maybe makes more sense for the component consumers to add those roles as they see fit. Would love to hear some thoughts about this though!

Copy link
Contributor

Choose a reason for hiding this comment

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

Reasoning makes sense to me — Grid is mostly about a visual grid layout, and it doesn't necessarily imply the semantics of role="grid".

For that reason, I'm not sure it makes much sense to add role="grid" to every test — on one hand, it allows us to use *ByRole queries, but on the other hand it feels a bit hacky.

I wonder if just passing a data-test-id to Grid is a better indication of our intensions in these tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

I never imagined I'd be reverting from using role to data-testid 😅 But yup, I agree that data-testid expresses our intent better. Updated in f47bd07

Copy link
Contributor

Choose a reason for hiding this comment

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

I just think that role needs to be used with intention :) In this case, if we were using role as a "proxy" for the Grid component, that was mostly a hack

It's tricky to test pure layout components (like grid) semantically, which is why I used to mostly rely on snapshot testing in those scenarios (i.e. I mainly want to test that markup and styles are correct, no need to check for semantics).

<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateColumns: 'repeat( 2, 1fr )',
gap: `calc( ${ CONFIG.gridBase } * 3 )`,
} );
} );

test( 'should render gap', () => {
const { container } = render(
<Grid columns={ 3 } gap={ 4 }>
render(
<Grid columns={ 3 } gap={ 4 } role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateColumns: 'repeat( 3, 1fr )',
gap: `calc( ${ CONFIG.gridBase } * 4 )`,
} );
} );

test( 'should render custom columns', () => {
const { container } = render(
<Grid columns={ 7 }>
render(
<Grid columns={ 7 } role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateColumns: 'repeat( 7, 1fr )',
} );
} );

test( 'should render custom rows', () => {
const { container } = render(
<Grid rows={ 7 }>
render(
<Grid rows={ 7 } role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateRows: 'repeat( 7, 1fr )',
} );
} );

test( 'should render align', () => {
const { container } = render(
<Grid align="flex-start">
render(
<Grid align="flex-start" role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
alignItems: 'flex-start',
display: 'grid',
} );
} );

test( 'should render alignment spaced', () => {
const { container } = render(
<Grid alignment="spaced">
render(
<Grid alignment="spaced" role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
alignItems: 'center',
justifyContent: 'space-between',
} );
} );

test( 'should render justify', () => {
const { container } = render(
<Grid justify="flex-start">
render(
<Grid justify="flex-start" role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
justifyContent: 'flex-start',
} );
} );

test( 'should render isInline', () => {
const { container } = render(
<Grid columns={ 3 } isInline>
render(
<Grid columns={ 3 } isInline role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'inline-grid',
gridTemplateColumns: 'repeat( 3, 1fr )',
} );
} );

test( 'should render custom templateColumns', () => {
const { container } = render(
<Grid templateColumns="1fr auto 1fr">
render(
<Grid templateColumns="1fr auto 1fr" role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateColumns: '1fr auto 1fr',
} );
} );

test( 'should render custom templateRows', () => {
const { container } = render(
<Grid templateRows="1fr auto 1fr">
render(
<Grid templateRows="1fr auto 1fr" role="grid">
<View />
<View />
<View />
</Grid>
);

expect( container.firstChild ).toHaveStyle( {
expect( screen.getByRole( 'grid' ) ).toHaveStyle( {
display: 'grid',
gridTemplateRows: '1fr auto 1fr',
} );
Expand Down