mattermost / mattermost-plugin-gitlab

GitLab plugin for Mattermost
Apache License 2.0
141 stars 85 forks source link

[MM-204] Fixed issues/enhancements in link tooltip component #457

Closed raghavaggarwal2308 closed 8 months ago

raghavaggarwal2308 commented 9 months ago

Summary

  1. Removed the "see more" link.
  2. Fixed branch name overflow.
  3. Added limit to title and description.
  4. Displayed proper icon for a closed PR.
  5. Updated the icons to match with the icons on gitlab.com.

Screenshot:

image

What to test:

  1. The "see more" link is not visible in the tooltip.
  2. The branch, label, title, and description are not overflowing.
  3. Proper icons are displayed for each type of issue/PR.
  4. Displayed icons are matching with the icons on Gitlab.
  5. Updated check to match the URL hostname instead of complete URL with protocol.
Steps to test:
  1. Connect your GitLab account.
  2. Hover over a GitLab PR/issue link in a channel.

Ticket Link

Fixes: https://community.mattermost.com/core/pl/rybgwy37xibpjgsqjrd8yxkkbo

codecov[bot] commented 9 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 33.43%. Comparing base (175c61d) to head (c5f231b). Report is 1 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #457 +/- ## ========================================== + Coverage 33.38% 33.43% +0.04% ========================================== Files 22 22 Lines 4014 4008 -6 ========================================== Hits 1340 1340 + Misses 2541 2535 -6 Partials 133 133 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

raghavaggarwal2308 commented 9 months ago

@matthewbirtch Updated

raghavaggarwal2308 commented 9 months ago

@avas27JTG Fixed the review comments mentioned by you

matthewbirtch commented 9 months ago

@matthewbirtch Updated

Thanks @raghavaggarwal2308 when you have a moment could provide an updated screenshot?

raghavaggarwal2308 commented 9 months ago

@matthewbirtch Updated

Thanks @raghavaggarwal2308 when you have a moment could provide an updated screenshot?

@matthewbirtch I have updated the screenshot in the PR description now 👍🏽

matthewbirtch commented 9 months ago

@matthewbirtch I have updated the screenshot in the PR description now 👍🏽

Looks good @raghavaggarwal2308, thanks. Is there any way we can truncate the description to be max 3 lines rather than having a few words overflow on to the 4th line?

raghavaggarwal2308 commented 9 months ago

@matthewbirtch I have updated the screenshot in the PR description now 👍🏽

Looks good @raghavaggarwal2308, thanks. Is there any way we can truncate the description to be max 3 lines rather than having a few words overflow on to the 4th line?

@matthewbirtch Yes, it is possible but it will be a little complex to do also it will make the component rendering a little slow. A hack for this would be to fix the height of the description area and hide the remaining content but in that case, we will not be able to add the ... at the end.

matthewbirtch commented 9 months ago

@matthewbirtch I have updated the screenshot in the PR description now 👍🏽

Looks good @raghavaggarwal2308, thanks. Is there any way we can truncate the description to be max 3 lines rather than having a few words overflow on to the 4th line?

@matthewbirtch Yes, it is possible but it will be a little complex to do also it will make the component rendering a little slow. A hack for this would be to fix the height of the description area and hide the remaining content but in that case, we will not be able to add the ... at the end.

Ok, I'm not sure if this is cross-browser compatible, but there is a line-clamp property that works for webkit. -webkit-line-clamp: 3;

raghavaggarwal2308 commented 9 months ago

@matthewbirtch These are the versions for browser compatibility of this property. If these look good to you we can make the change image

matthewbirtch commented 9 months ago

@matthewbirtch These are the versions for browser compatibility of this property. If these look good to you we can make the change

I think we should be good, since these are minimum requirements we currently publish:

image

I believe we are using this CSS property in the webapp already in a few other locations anyway.

raghavaggarwal2308 commented 9 months ago

@matthewbirtch Updated the CSS

matthewbirtch commented 9 months ago

@matthewbirtch Updated the CSS

Thanks @raghavaggarwal2308. Could you share an updated screenshot again?

raghavaggarwal2308 commented 9 months ago

@matthewbirtch Here is the updated screenshot image

matthewbirtch commented 9 months ago

Thanks @raghavaggarwal2308 looks good to me

raghavaggarwal2308 commented 9 months ago

@mickmister Fixed the review comments added by you

raghavaggarwal2308 commented 9 months ago

@AayushChaudhary0001 The issue is fixed now. Please re-review.

AayushChaudhary0001 commented 9 months ago

Tested and working fine now for the positioning of icons also, LGTM. Approved