-
Notifications
You must be signed in to change notification settings - Fork 6
Feature 2244 avatar click #2248
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.
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
Conversation
| import { useLocation, Link } from 'react-router-dom'; | ||
| import { AppContext } from '../../context/AppContext'; | ||
| import { getAvatarURL } from '../../api/api'; | ||
| import AvatarMenu from '@mui/material/Menu'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clicking the avatar no longer displays a menu.
| return links; | ||
| }; | ||
|
|
||
| const handleClick = event => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now handled by a Link component.
| setFeedbackOpen(false); | ||
| }; | ||
|
|
||
| const closeAvatarMenu = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no longer an avatar menu to close.
| aria-haspopup="true" | ||
| onClick={handleToggle} | ||
| > | ||
| <Link to={`/profile/${id}`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The avatar is now wrapped in a Link that goes to the Profile page.
| > | ||
| <Link to={`/profile/${id}`}> | ||
| <Avatar | ||
| onClick={handleClick} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The click is now handled by the Link that wraps this Avatar component.
| textDecoration: 'none' | ||
| }} | ||
| /> | ||
| <AvatarMenu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No more avatar menu.
| <div | ||
| aria-haspopup="true" | ||
| <a | ||
| href="/profile/undefined" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the reason this contains "undefined" is that in the test we don't have a user id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Menu.test.jsx currently I see us taking snapshots, but I don't see us making any assertions yet other than something changed. In this case, the ${id} is set at runtime based on the user profile. If we want to test various render scenarios in our tests, and not just that it renders, I think we may find some room for improvement in our test suites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test to verify that the Link around the Avatar uses the id value.
| <div | ||
| aria-haspopup="true" | ||
| <a | ||
| href="/profile/undefined" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the previous comment.
| <div | ||
| aria-haspopup="true" | ||
| <a | ||
| href="/profile/undefined" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the previous comment.
vhscom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
vhscom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love seeing some examples of how we're component testing. I saw we also have Testing Library, which can be useful as its API surface tends to nudge implementations towards semantic markup and use of ARIA where it matters most (e.g. component suspense states).
| const link = container.querySelector('header > a'); | ||
| const href = link.getAttribute('href'); | ||
| const lastIndex = href.lastIndexOf('/'); | ||
| const id = href.substring(lastIndex + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to grab the id could be to split the string into an array and pop off the last element:
const id = href.split('/').pop()
No description provided.