Uh oh!
There was an error while loading. Please reload this page.
[UnderlineNav2]: Increase the unit test coverage - #2506
Conversation
🦋 Changeset detectedLatest commit: 41e17ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
af846cf to
41ce49eCompare
joshblack
left a comment
There was a problem hiding this comment.
Looks good! Just had a couple questions around conventions, was curious what your thoughts were 👀
| }) | ||
| it('should move focus to the next/previous item on the list with the tab key', () => { | ||
| const {getByText} = render(<ResponsiveUnderlineNav />) | ||
| const item = getByText('Code').closest('a') as HTMLAnchorElement |
There was a problem hiding this comment.
When do you think it would be better to do getByText('Code').closest('a') versus getByRole('link', { name: 'Code' })
There was a problem hiding this comment.
Hmm 🤔 I would use them interchangeable but probably not the best. I can't think of any case right now where using the element.closest('a') will be the best, maybe for some reason if we cannot capture the name of the link? Do you have any example? That said, getByRole is more declarative so I'll opt in using this, thank for pointing this out :)
| // Focus first item | ||
| item.focus() | ||
| // Press down arrow | ||
| fireEvent.keyDown(item, tab) |
There was a problem hiding this comment.
When do you think would be best to use fireEvent versus user.tab() from user events? e.g. https://testing-library.com/docs/user-event/convenience/#tab
There was a problem hiding this comment.
I didn't really know the differences! 😿 Thanks for pointing this out! I got the habit of using fireEvent but I just read about both now and using userEvents makes better sense here because we want to simulate the exact behaviour of user pressing the tab key rather than just firing the actual keydown event. TIL that userEvents might include some other chain of events before the actual keydown fireEvent.
To answer your question, using fireEvent might be a better use probably if we need a different set of chain events before firing the keydown event. For example if we want to test tab key firing without focusing it maybe
broccolinisoup
commented
Nov 7, 2022
@joshblack Thanks so much for your review, great points! I pushed another commit, I'd appreciate if you could have another look. Thanks! |
e06cdc8 to
74b5376Compare
Adding unit tests to UnderlineNav
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.