Skip to content
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

fix(misc): show full length of agenda item text when hovering #10251

Merged
merged 4 commits into from
Sep 19, 2024

Conversation

tianrunhe
Copy link
Contributor

@tianrunhe tianrunhe commented Sep 17, 2024

Demo

2024-09-17 10 22 13

2024-09-17 10 23 13

Testing scenarios

  • Agenda Item at team dash

    • Hover an agenda item at team dash page
    • See full length of the text in the tooltip
    • Delete/Pin/Unpin still works
  • Agenda Item in check-in meeting

    • Hover an agenda item in a check-in meeting
    • See full length of the text in the tooltip
    • Delete/Pin/Unpin still works

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@tianrunhe tianrunhe added the Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved label Sep 17, 2024
Copy link
Contributor

@nickoferrall nickoferrall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You assigned me to the PR but I assume you meant reviewer, so here is my review 😄

Ideally, I think we should only show the tooltip when you can't see the entire agenda item. Most of the time, you can read the full agenda item, so you wouldn't want to see a tooltip: https://www.loom.com/share/498faf0911d24118a8fab147678cd953

packages/client/components/MeetingSubnavItem.tsx Outdated Show resolved Hide resolved
@@ -238,7 +238,9 @@ const AgendaItem = (props: Props) => {
/>
</AgendaItemStyles>
{tooltipPortal(
pinned ? `Unpin "${content}" from every check-in` : `Pin "${content}" to every check-in`
pinned
? `Unpin this agenda topic from every check-in`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 why remove the "content" bit out of interest? The tooltip updates the position so we can always see it. I prefer being able to see the content of the tooltip I'm pinning so I'm sure I'm pinning the right one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it redundant & too verbose? I'm trying to follow a "industry standard" (if that's a thing😂). For example, in Arc tab management:

Screenshot 2024-09-18 at 10 39 04

Notice here it says "archive this tab", not "archive 'GitHub'"

@tianrunhe
Copy link
Contributor Author

You assigned me to the PR but I assume you meant reviewer, so here is my review 😄

Sorry for randomly assigning work to you, this is what happened when you go on a vacation and back 🤣!
Thanks for reviewing this!

Copy link
Contributor

@nickoferrall nickoferrall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tianrunhe tianrunhe merged commit 89661a7 into master Sep 19, 2024
6 checks passed
@tianrunhe tianrunhe deleted the fix/agendaItemTooltips branch September 19, 2024 17:07
@github-actions github-actions bot mentioned this pull request Sep 24, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/s Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants