[GH-255] Add tooltip for pull request and issue links #258

fedealconada · 2020-05-05T12:06:37Z

Summary

This PR adds a tooltip for pull request and issue links

Ticket Link

Fixes #255

hanzei

This is AMAZING 🎉 I'm really looking forward to seeing this merged

webapp/src/components/link_tooltip/link_tooltip.jsx

hanzei · 2020-05-05T12:59:13Z

webapp/src/components/link_tooltip/tooltip.css

+    -webkit-hyphens: auto;
+    -ms-hyphens: auto;
+    hyphens: auto;
+    max-height: 150px;


A larger texts gets cut of with this approach. Is there a more elegant way to cut of the text, if it gets to long? E.g. after X words.

Well... actually, I gave this a though and decided to do it like that. The thing is, the scrolling is enabled, so you should be able to scroll down there but I have hidden the scrollbar because I thought it was more "elegant" not to show it, but it might seem a bit weird.
Either I could show the scrollbar or I could play around with the 'react-markdown' so it only renders an amount of HTML tags... There are several options in which this could be done... I could also just convert the whole text to one only
with no format and add ellipsis on overflow, i don't know....
I would say that, because of the scope of this issue (which was not actually including the pr/issue description on the tooltip), I would rather decide between keeping it like this or un-hiding the scrollbar. Thoughts?

I'm wondering how much a scrollbar would be actually used. The tooltip is mostly "for a quick glance" and if a user wants for information, the link to the issue/pr is right there.

@asaadmahmood I would like to hear your toughs on this. Is the current solution acceptable to you?

I feel it could be useful... Even though it's a quick glance, sometimes we get a bit lazy about having to open the link, wait the page to load, etc etc. Imagine that maybe you know there's a useful link inside the description that you know is placed at the bottom of it... just scrolling will help you out... But yeah, up to others opinions :)

@hanzei Sorry for replying late guys. I don't think a scrollbar should be needed here, we can just have a max-height or max number or elements, without it cutting off content.

I would also be willing to add a "See more" link above the labels which would just open the PR (similar to the title link) to be very obvious.

it currently has a max-height, which cuts the text off and that's why I've added the ability to scroll (just that I've hidden the scrollbar, but you are allowed to scroll) @asaadmahmood I'm not so sure which is the solution you propose.
Regarding the "See more" link, no problem!

server/api.go

hanzei · 2020-05-05T19:44:52Z

@asaadmahmood A couple of screenshots:

webapp/src/components/link_tooltip/link_tooltip.jsx

asaadmahmood

LGTM, apart from that other change.

hanzei

My outstanding concerns are not blocking for this PR can be addressed within #252

larkox

Fantastic PR! I am looking forward to see it deployed!
Just one small thing, apart of what has been said by other reviewers.

webapp/src/components/link_tooltip/link_tooltip.jsx

larkox

LGTM, thanks!

DHaussermann · 2020-05-14T17:28:15Z

@hanzei i If you have some time, could you please peer-test this PR using the peer testing process documented here: https://mattermost.atlassian.net/wiki/spaces/INT/pages/619937892/Peer+Testing+Process

codecov-io · 2020-05-16T05:53:09Z

Codecov Report

Merging #258 into master will decrease coverage by 0.24%.
The diff coverage is 5.26%.

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
- Coverage   19.69%   19.44%   -0.25%     
==========================================
  Files          10       10              
  Lines        2178     2216      +38     
==========================================
+ Hits          429      431       +2     
- Misses       1725     1761      +36     
  Partials       24       24

Impacted Files	Coverage Δ
server/api.go	`6.63% <5.26%> (-0.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fcc0ad...0ef70f4. Read the comment docs.

hanzei

Scope

Tooltips work for open and closed
Tooltips works for open, merged and closed PRs

Testing notes

When a user is connected to GitHub, all five tooltip types are shown correctly.
When a user is not connected to GitHub, a JS warning will be shown on page load.

@fedealconada Could you please look into this?

Follow-up concerns

None

webapp/src/components/link_tooltip/link_tooltip.jsx

fedealconada · 2020-05-16T18:55:57Z

@hanzei I've just fixed the warning issue, check the changes.

…-plugin-github into mattermostGH-255

hanzei

Thanks for fixing the warning 👍

The CI complains a about these changes:

  4:1   error  There should be at least one empty line between import groups  import/order
 
  5:1   error  There should be at least one empty line between import groups  import/order
 
  8:33  error  'ownProps' is defined but never used                           no-unused-vars
 
  9:12  error  There should be no space after '{'                             object-curly-spacing
 
  9:64  error  There should be no space before '}'                            object-curly-spacing
 
  9:65  error  Missing semicolon                                              semi

Would you please fix these issue? After that the PR should be ready to go.

asaadmahmood · 2020-05-17T10:53:37Z

@hanzei @fedealconada Not going to go much into the code, but can we have screenshots of how it looks so I can approve/comment on it from a UX perspective?

fedealconada · 2020-05-17T12:10:29Z

@hanzei my bad, I forgot to run the linter. Changes are pushed now :) @asaadmahmood see below the screenshot, the only thing that has changed has been the addition of the "See more" button. Clicking on it opens up a new tab.

Hovering over the link looks like this:

asaadmahmood · 2020-05-17T13:23:31Z

@fedealconada Would it not be possible for us to just add an elipsis after x number of characters and then on a new line add See more?
I don't like it cutting the text like that.

asaadmahmood · 2020-05-17T14:23:28Z

@fedealconada Can we also vertically middle align the arrow here.

fedealconada · 2020-05-18T12:18:38Z

Hey @asaadmahmood, I agree, I don't like the text being cut off. I've been digging a bit into that. What I've managed to do is, instead of adding ellipsis, just hide the elements (check image below).

Thoughts?

asaadmahmood · 2020-05-18T12:19:40Z

@fedealconada This looks good to me, but are we sure that solution would work with different PR descriptions?

asaadmahmood · 2020-05-18T14:44:06Z

Discussed it with @fedealconada, we're going with the overflow trick for now, and may create an improvement issue later.

hanzei

Scope

Tooltips work for open and closed
Tooltips works for open, merged and closed PRs

Testing notes

When a user is connected to GitHub, all five tooltip types are shown correctly.
When a user is not connected to GitHub, no JS warning is shown. Also, no tooltip is shown.

Follow-up concerns

None

DHaussermann

Thanks for the testing help on this @hanzei. Just as a precaution, I also took a quick look on desktop in case you had not reviewed it.
No issues found.

Huge thanks @fedealconada for this PR! This is an great UX improvement!

add GET /issue and GET /pr endpoints

90d31a2

fedealconada requested a review from hanzei as a code owner May 5, 2020 12:06

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 5, 2020

add github tooltip on webapp

a9fc584

fedealconada force-pushed the GH-255 branch from 6984517 to a9fc584 Compare May 5, 2020 12:08

hanzei requested changes May 5, 2020

View reviewed changes

add changes from PR review

33cf2b3

hanzei mentioned this pull request May 5, 2020

Compute label foreground colour using WCAG rules #130

Closed

hanzei linked an issue May 5, 2020 that may be closed by this pull request

Compute label foreground colour using WCAG rules #130

Closed

hanzei added the 1: UX Review Requires review by a UX Designer label May 5, 2020

hanzei requested a review from asaadmahmood May 5, 2020 19:36

fedealconada added 2 commits May 5, 2020 22:17

move to utils and use it to fix sidebar labels

a65f718

remove unnecessary classes and fix format

9fe3e01

hanzei reviewed May 6, 2020

View reviewed changes

webapp/src/components/link_tooltip/link_tooltip.jsx Outdated Show resolved Hide resolved

fix typo

8e842f6

asaadmahmood approved these changes May 12, 2020

View reviewed changes

add See more link to github tooltip

a060193

hanzei requested a review from larkox May 12, 2020 12:58

hanzei removed the 1: UX Review Requires review by a UX Designer label May 12, 2020

hanzei approved these changes May 13, 2020

View reviewed changes

hanzei added this to the v1.0.0 milestone May 13, 2020

larkox requested changes May 13, 2020

View reviewed changes

webapp/src/components/link_tooltip/link_tooltip.jsx Show resolved Hide resolved

avoid calling API if data already set

ffd8e73

larkox approved these changes May 13, 2020

View reviewed changes

larkox removed the 2: Dev Review Requires review by a core committer label May 13, 2020

larkox requested a review from DHaussermann May 13, 2020 14:23

Merge branch 'master' into mattermostGH-255

9c00d07

hanzei requested changes May 16, 2020

View reviewed changes

webapp/src/components/link_tooltip/link_tooltip.jsx Show resolved Hide resolved

hanzei mentioned this pull request May 16, 2020

If a plugin registered a custom tooltip, clicking hashtags to have no effect mattermost/mattermost#14499

Closed

add connected checker

9650f7f

Merge branch 'mattermostGH-255' of github.com:fedealconada/mattermost…

cfb4163

…-plugin-github into mattermostGH-255

hanzei requested changes May 17, 2020

View reviewed changes

run linter

c155d64

remove scrollbar and keep overflow hidden

c148406

fedealconada force-pushed the GH-255 branch from 0ef70f4 to c148406 Compare May 18, 2020 12:46

Merge remote-tracking branch 'origin/master' into mattermostGH-255

f66f592

hanzei approved these changes May 18, 2020

View reviewed changes

DHaussermann approved these changes May 19, 2020

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request QA Review Done PR has been approved by QA and removed 3: QA Review Requires review by a QA tester labels May 19, 2020

hanzei merged commit b124fd0 into mattermost:master May 19, 2020

levb mentioned this pull request May 19, 2020

Bump version to v1.0.0 #274

Merged

larkox mentioned this pull request May 25, 2020

Use official GitHub icons #268

Closed

jfrerich mentioned this pull request Jun 17, 2020

[HARD] Add Tooltips for each bookmark in the /bookmarks view jfrerich/mattermost-plugin-bookmarks#121

Open

aaronrothschild mentioned this pull request Oct 2, 2020

Add the ability to Preview a Gitlab issue on hover within Mattermost mattermost/mattermost-plugin-gitlab#212

Open

hanzei mentioned this pull request Mar 15, 2022

Backport github plugin improvements mattermost/mattermost-plugin-gitlab#61

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-255] Add tooltip for pull request and issue links #258

[GH-255] Add tooltip for pull request and issue links #258

fedealconada commented May 5, 2020 •

edited by hanzei

Loading

hanzei left a comment

hanzei May 5, 2020

fedealconada May 5, 2020

hanzei May 5, 2020

fedealconada May 5, 2020

asaadmahmood May 12, 2020

fedealconada May 12, 2020

hanzei commented May 5, 2020

asaadmahmood left a comment •

edited

Loading

hanzei left a comment

larkox left a comment

larkox left a comment

DHaussermann commented May 14, 2020

codecov-io commented May 16, 2020 •

edited

Loading

hanzei left a comment •

edited

Loading

fedealconada commented May 16, 2020

hanzei left a comment

asaadmahmood commented May 17, 2020

fedealconada commented May 17, 2020

asaadmahmood commented May 17, 2020

asaadmahmood commented May 17, 2020

fedealconada commented May 18, 2020

asaadmahmood commented May 18, 2020

asaadmahmood commented May 18, 2020

hanzei left a comment

DHaussermann left a comment

[GH-255] Add tooltip for pull request and issue links #258

[GH-255] Add tooltip for pull request and issue links #258

Conversation

fedealconada commented May 5, 2020 • edited by hanzei Loading

Summary

Ticket Link

hanzei left a comment

Choose a reason for hiding this comment

hanzei May 5, 2020

Choose a reason for hiding this comment

fedealconada May 5, 2020

Choose a reason for hiding this comment

hanzei May 5, 2020

Choose a reason for hiding this comment

fedealconada May 5, 2020

Choose a reason for hiding this comment

asaadmahmood May 12, 2020

Choose a reason for hiding this comment

fedealconada May 12, 2020

Choose a reason for hiding this comment

hanzei commented May 5, 2020

asaadmahmood left a comment • edited Loading

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

larkox left a comment

Choose a reason for hiding this comment

DHaussermann commented May 14, 2020

codecov-io commented May 16, 2020 • edited Loading

Codecov Report

hanzei left a comment • edited Loading

Choose a reason for hiding this comment

Scope

Testing notes

Follow-up concerns

fedealconada commented May 16, 2020

hanzei left a comment

Choose a reason for hiding this comment

asaadmahmood commented May 17, 2020

fedealconada commented May 17, 2020

asaadmahmood commented May 17, 2020

asaadmahmood commented May 17, 2020

fedealconada commented May 18, 2020

asaadmahmood commented May 18, 2020

asaadmahmood commented May 18, 2020

hanzei left a comment

Choose a reason for hiding this comment

Scope

Testing notes

Follow-up concerns

DHaussermann left a comment

Choose a reason for hiding this comment

fedealconada commented May 5, 2020 •

edited by hanzei

Loading

asaadmahmood left a comment •

edited

Loading

codecov-io commented May 16, 2020 •

edited

Loading

hanzei left a comment •

edited

Loading