Cap tag list of person hover cards #1920
Conversation
| hiddenTags | ||
| .slice(0, 3) | ||
| .map((tag) => tag.title) | ||
| .join(', ') + (hiddenTags.length > 3 ? ', ...' : ''); |
There was a problem hiding this comment.
I'm curious about the choice to cap the amount of tag titles visible in the tooltip. I would expect that the tooltip showed all the titles of all the tags that are not visible - so that the number that indicates how many hidden tags there are is the amount of tag titles that are shown in the tooltip.
How do you think about this?
There was a problem hiding this comment.
I must have turned this around in my head, somehow! Yes, it should display the number of remaining ones, let me fix that real quick.
There was a problem hiding this comment.
Alright, I think I misunderstood your question:
The reason I cap the number of titles shown in the tooltip is that otherwise it could be potentially massive. Which was the problem we tried to avoid for the hover card, so I assumed something similar here. That said, it could certainly by higher than just 3 titles.
There was a problem hiding this comment.
I ran this by Josef and Victor on the design team and after a bit of discussions the direction they want this to go is to cap at 10 visible tags instead of 3, remove the tooltip on the "+X tags" and instead have that chip link to the person's profile page (where all tags are visible). Sounds ok?
| isGrouped: boolean; | ||
| onUnassignTag?: (tag: ZetkinTag) => void; | ||
| tags: ZetkinTag[]; | ||
| }> = ({ tags, isGrouped, onUnassignTag }) => { | ||
| }> = ({ cap = Infinity, isGrouped, onUnassignTag, tags }) => { |
There was a problem hiding this comment.
I have never seen or used Infinity before, so I read up on it, cool thing!
| const cappedTags = tags.slice(0, cap); | ||
| const hiddenTags = tags.slice(cap); | ||
| const isCapped = tags.length > cappedTags.length; | ||
| const tooltipTitle = |
There was a problem hiding this comment.
I really like how clearly named these are and that you saved each thing in a constant instead of just using them inline!
There was a problem hiding this comment.
Writing it here also to make it more clear:
I ran the matter of the tooltip by Josef and Victor on the design team and after a bit of discussions the direction they want this to go is to cap at 10 visible tags instead of 3, remove the tooltip on the "+X tags" and instead have that chip link to the person's profile page (where all tags are visible). :D
| import { ZetkinTag } from 'utils/types/zetkin'; | ||
| import { Box, Theme, Typography } from '@mui/material'; |
There was a problem hiding this comment.
This is annoying but this line of import is external, so it belongs to the first group of imports. We have a pending PR for a linting rule that will do this for us eventually, but until then this needs to be moved up to the first import group :)
There was a problem hiding this comment.
Go right ahead and fix this if you want, it saves me a little brainpower!
| chip: { | ||
| borderColor: theme.palette.grey[500], | ||
| borderRadius: '1em', | ||
| borderWidth: '1px', | ||
| color: theme.palette.text.secondary, | ||
| cursor: 'default', | ||
| cursor: ({ clickable }) => (clickable ? 'pointer' : 'default'), |
Correct order of imports.
Description
Caps the maximum number of tags shown when hovering a person.
The same cap is applied for each group of tags individually.
Changes
Related issues
Resolves #1767