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.

Sign up for GitHub

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

Jump to bottom

[MM-166] Fix issue: decription not visible in tooltip and removed "see more" link #735

Merged
merged 5 commits into from
Feb 7, 2024
Merged

[MM-166] Fix issue: decription not visible in tooltip and removed "see more" link #735

merged 5 commits into from
Feb 7, 2024

Conversation

raghavaggarwal2308
Copy link
Contributor

Summary

  • Fix issue: description not visible in tooltip.
  • Removed the see more link in the tooltip.
  • Set a max length for the tooltip description.

Screenshot

image

What to test

  • Description and other contents are visible in the tooltip properly.
  • "See more" link is removed
  • When the description is very long it's getting trimmed.
Steps to test:
  1. Connect your GitHub account
  2. Hover over a PR/Issue link.

Ticket Link

Fixes #734

mickmister
mickmister reviewed Jan 30, 2024
View reviewed changes
webapp/src/components/link_tooltip/link_tooltip.jsx Outdated Show resolved Hide resolved
webapp/src/components/link_tooltip/tooltip.css Outdated
Comment on lines 69 to 71
max-height: 150px;
line-height: 1.25;
overflow: hidden;
Copy link
Member

@mickmister mickmister Jan 30, 2024 •
edited

Choose a reason for hiding this comment

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

What are the reasons/risks for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister This was being used to trim the description when it was too long. We modified the logic to set a max length to the description now.

Copy link
Member

Choose a reason for hiding this comment

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

It's still possible that it overflows though right? Is there any risk in keeping overflow: hidden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister I don't think it will overflow but there is no risk in keeping it so I have reverted the change

Copy link
Member

Choose a reason for hiding this comment

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

Was it the overflow: hidden that was making it so the description wasn't being shown at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister No, the component ReactMarkdown was not used properly.

webapp/src/components/link_tooltip/link_tooltip.jsx
@@ -118,10 +120,7 @@ export const LinkTooltip = ({href, connected, show, theme}) => {
<span>{'#' + data.number}</span>
</a>
<div className='markdown-text mt-1 mb-1'>
<ReactMarkdown
source={data.body}
linkTarget='_blank'
Copy link
Member

@mickmister mickmister Jan 30, 2024 •
edited

Choose a reason for hiding this comment

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

What is the risk in removing this linkTarget prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister I misunderstood the use of this prop. Added it back

mickmister
mickmister reviewed Jan 30, 2024
View reviewed changes
Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

Great job @raghavaggarwal2308 👍 Looks good, just a few comments for discussion

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2608197) 15.78% compared to head (ff27370) 15.78%.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #735   +/-   ##
=======================================
  Coverage   15.78%   15.78%           
=======================================
  Files          15       15           
  Lines        5770     5770           
=======================================
  Hits          911      911           
  Misses       4817     4817           
  Partials       42       42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister
mickmister reviewed Feb 1, 2024
View reviewed changes
webapp/src/components/link_tooltip/link_tooltip.jsx Outdated Show resolved Hide resolved
webapp/src/components/link_tooltip/tooltip.css Outdated
Comment on lines 69 to 71
max-height: 150px;
line-height: 1.25;
overflow: hidden;
Copy link
Member

Choose a reason for hiding this comment

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

It's still possible that it overflows though right? Is there any risk in keeping overflow: hidden?

hanzei
hanzei approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

LGTM

webapp/src/components/link_tooltip/tooltip.css Outdated Show resolved Hide resolved
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Feb 2, 2024
@hanzei hanzei added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer labels Feb 6, 2024
AayushChaudhary0001
AayushChaudhary0001 approved these changes Feb 7, 2024
View reviewed changes
Copy link

@AayushChaudhary0001 AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

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

This PR has been tested for the following scenarios:-

  • Added description in the tooltip.
  • Removed the "See more" link from the tooltip.
  • Text gets trimmed if its length is too much.

The PR is working fine for the above conditions, LGTM. Approved.

@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Feb 7, 2024
@mickmister mickmister merged commit 1d43e9e into mattermost:master Feb 7, 2024
9 checks passed
@raghavaggarwal2308 raghavaggarwal2308 mentioned this pull request Feb 9, 2024
Merged
@mickmister mickmister mentioned this pull request Feb 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mickmister mickmister mickmister approved these changes

@hanzei hanzei hanzei approved these changes

@AayushChaudhary0001 AayushChaudhary0001 AayushChaudhary0001 approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue/PR descriptions not showing in link tooltips
5 participants
@raghavaggarwal2308 @codecov-commenter @mickmister @hanzei @AayushChaudhary0001