Update Profile TrackSwitcher Icon onChange #830
Conversation
|
@neenjaw Thanks! Want to throw a test in for it? |
track switcher implementation not tested in depth because previously covered by `useDropdown.test.js`
neenjaw
requested review from
ErikSchierboom,
iHiD and
kntsoriano
as code owners
| { | ||
| files: ['*.test.ts', '*.test.tsx'], | ||
| rules: { | ||
| '@typescript-eslint/no-empty-function': 'off', |
There was a problem hiding this comment.
it is sometimes easy to use an empty function for testing without creating a complex requirement
| files: ['*.test.ts', '*.test.tsx'], | ||
| rules: { | ||
| '@typescript-eslint/no-empty-function': 'off', | ||
| '@typescript-eslint/no-unused-vars': 'off', |
There was a problem hiding this comment.
just my opnion here but in test files I don't think this matters too much. Maybe downgrading it to a warning rather than an error?
| { | ||
| files: ['*.ts', '*.tsx'], | ||
| rules: { | ||
| 'react/prop-types': 'off', |
There was a problem hiding this comment.
prop-types aren't used in typescript. I think this is raising an error just because we have a mix of js and ts in the project
|
@neenjaw Thanks! |
| const button = screen.getByLabelText('Open the track switcher') | ||
|
|
||
| await act(async () => { | ||
| await fireEvent.click(button) |
There was a problem hiding this comment.
I usually use userEvent.click instead of fireEvent.click. Is there any advantage to use fireEvent instead?
There was a problem hiding this comment.
I use fireEvent because that's what I know, but looking for a reference to think points out that using userEvent might be a better way to go about this. Reference: testing-library/eslint-plugin-testing-library#162 (comment)
There was a problem hiding this comment.
@neenjaw Want to change or should I merge? :)
There was a problem hiding this comment.
They are essentially the same thing, so this works and covers it, but if I'm in this piece of code again I would change it. I think good to merge
| @@ -0,0 +1,66 @@ | |||
| import { RepresenterFeedback } from '../../../app/javascript/components/mentoring/session/RepresenterFeedback' | |||
There was a problem hiding this comment.
Nice :) I should do something like this from now on.
Previous to this patch, the track switcher on the profile page did not update the icon based on the track:
With this patch, it updates when the value changes (1 line fix)