feat(navigation) - add inert attribute to NavExpandable #11749
Conversation
|
Preview: https://patternfly-react-pr-11749.surge.sh A11y report: https://patternfly-react-pr-11749-a11y.surge.sh |
|
@thatblindgeye should I also include a cypress test? There was some doubt regarding it in the issue. |
There was a problem hiding this comment.
One file comment below. As for your question, we could probably handle testing in the unit tests. Ideally off the top of my head we'd want the following tests:
- check that inert is on the
<section>element by default - check that inert is NOT on the
<section>when isExpanded is true
Then utilizing userEvents:
- click the expandable toggle to expand it, then
tab()and assert that first item is focused - click the expandable toggle to collapse it, then
tab()and assert that the first item within that section doesn't have focus
We'd also want these to be in a new NavExpandable.test.tsx file. If you have any questions or run into any issues just let me know
|
Thanks for the amazing instructions, will start working on the unit test. |
Mash707
force-pushed
the
navigation-add-inert-attribute
branch
from
3d1172f to
5992339
Compare
@thatblindgeye I have added tests regarding these two. Let me know what you think |
There was a problem hiding this comment.
Changes for the current tests below. I'm also thinking that the other tests I mentioned might make more sense as Cypress integration tests than unit tests. We could use the existing Nav cypress demos to add these tests on, let me know if you have any thoughts or questions about that.
| const props = { | ||
| items: [ | ||
| { to: '#link1', label: 'Link 1' }, | ||
| { to: '#link2', label: 'Link 2' }, | ||
| { to: '#link3', label: 'Link 3' } | ||
| ] | ||
| }; |
There was a problem hiding this comment.
We can remove this object
| ] | ||
| }; | ||
|
|
||
| describe('NavExpandable', () => { |
There was a problem hiding this comment.
Currently I think we rarely use the describe block in our revamped tests; I personally try to use it more to separate out lengthy amounts of tests that can be grouped together. For now I think we can remove the describe block and just have each test on its own.
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
| }); |
There was a problem hiding this comment.
We shouldn't need this here
| import { NavItem } from '../NavItem'; | ||
| import { Nav, NavContext } from '../Nav'; | ||
| import { NavList } from '../NavList'; |
There was a problem hiding this comment.
So for the purposes of this set of unit tests, we don't need any of these other Nav components. We really only want to test that NavExpandable behaves/works the way we expect. Sometimes that does involve importing other components/creating mocks, but for the tests relevant to this PR we can get away wthout them.
There was a problem hiding this comment.
Oh great, reduced the code by a lot. I am new to writing tests so learning a lot.
| @@ -0,0 +1,60 @@ | |||
| import { render, screen } from '@testing-library/react'; | |||
| import userEvent from '@testing-library/user-event'; | |||
There was a problem hiding this comment.
Since this isn't being used let's remove this import
There was a problem hiding this comment.
I was testing some things out with the remaining two tests, missed removing it before pushing changes.
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| test('check that inert is on the section element by default', () => { |
There was a problem hiding this comment.
For the 2 tests here, with the above comments taken into account, we can slim down the code written:
- Let's update the test names similar to how other revamped tests we have, along the lines of "Renders with the inert attribute by default" and "Does not render with the intert attribute when isExpanded is true".
- For the
rendermethod, let's just pass aNavExpandablewith a title prop passed in - For the
expect, we can use thegetByLabelTextmatcher using the string you pass to thetitleprop and then assert that the inert attribute is/isn't there.
There was a problem hiding this comment.
Got it, will make the necessary changes.
I also think Cypress tests would be better since we have interactions like clicking and tabbing. I'll work on a draft and would be happy to get it reviewed. |
| }); | ||
|
|
||
| test('Does not render with the inert attribute when isExpanded is true', () => { | ||
| render(<NavExpandable id="grp-1" title="NavExpandable" isExpanded={true}></NavExpandable>); |
There was a problem hiding this comment.
Tiny nit, for these boolean props we don't need to pass a truthy value in like this, as just isExpanded on its own will suffice.
Closes #11732