Create custom GraphQL types for UserPage and UserList - #279
Conversation
This is the commit with the strange unused 'import App' that when removed breaks the tests.
Update error and loading state handlers for UserPage.
This test will check that when a listElement is clicked, the user is redirected to the user page which is at route '/user'.
Update logic for rendering in UserList.js
- gcr.io/track-compliance/api:master-596f9ee - gcr.io/track-compliance/frontend:master-91c4299 [ci skip]
Add these types to the query type as well.
Update UserList.js to bind data based on new query format. Update /graphql/queries to user new query format based on custom type. Updated tests to user new query format.
Bind admin value to checkbox.
Ethanljf
left a comment
Ethanljf
left a comment
There was a problem hiding this comment.
UserPage and tests look good! Great stuff! 🍠
nsdeschenes
left a comment
nsdeschenes
left a comment
There was a problem hiding this comment.
Looks good, just checkout my comments
Use new subtype in query. Update test and component to use new query.
| import { UPDATE_PASSWORD } from '../graphql/mutations' | ||
|
|
||
| describe('<UserPage />', () => { | ||
| afterEach(cleanup) |
There was a problem hiding this comment.
It is no longer required to call cleanup in the afterEach.
| it('renders without error', () => { | ||
| act(() => { | ||
| render( | ||
| <MockedProvider mocks={mocks} addTypename={false}> | ||
| <MemoryRouter initialEntries={['/']}> | ||
| <ThemeProvider theme={theme}> | ||
| <I18nProvider i18n={i18n}> | ||
| <UserPage /> | ||
| </I18nProvider> | ||
| </ThemeProvider> | ||
| </MemoryRouter> | ||
| </MockedProvider>, | ||
| ) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
If you asserted that the content you provided in the mocks was actually getting rendered this would be way better.
Since you are supplying the values in the mocks you can safely use getByText to find them in the output:
const { getByText } = render(...) and then await waitFor(() => getByText('sometext')).
| newTag: master-4301b10 | ||
| - name: gcr.io/track-compliance/frontend | ||
| newTag: master-7ee5ba8 | ||
| newTag: master-91c4299 |
There was a problem hiding this comment.
This doesn't really belong in this commit.
| </I18nProvider> | ||
| </ThemeProvider>, | ||
| ) | ||
| expect(container).toBeDefined() |
There was a problem hiding this comment.
container will always be defined here as it's the <div> within which the component is rendered.
| return ( | ||
| <PseudoBox | ||
| key={edge.node.id} | ||
| role="userCard" |
There was a problem hiding this comment.
The role attribute refers to an ARIA role. There are fixed number of them defined in the standards and relied on by screen readers and other assistive technology. Most HTML elements have an implicit role (so a <button /> has an implied role=button.
PseudoBox here is probably a <div> by default, and the non-standard role of userCard is being added to it can be found by Testing Library's getAllByRole function later. I don't know if this would have actively negative consequences for assitive tech users (given that that role isn't a real ARIA role), but it's not a good idea.
Breaking these components down smaller and favouring composition makes it easier to test. Smaller components can spread props directly onto certain elements they render, allowing the use of getByTestId for really simple selections. This would be a better strategy to the way roles are being used here.
There was a problem hiding this comment.
@sleepycat Mike, thanks for the awesome feedback!!! Do you think a "userCard component would be better here???
Update UserList and UserList.test to accomodate these changes. Update UserCard Test. Add color check tests back to UserCard.test.js
|
@sleepycat please see my latest commit where I attempt to resolve all of the issues your found at the end of last week. Would love to chat about it more! 😄 |
4 participants
Additions