Summary

Add a smart tooltip to show descriptions in the diffviewer

Review Request #7256 — Created April 27, 2015 and submitted May 15, 2015, 11:50 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

dvcs

Bugs

Depends On

Commit

fe4fb6d...

Reviewers

Groups

reviewboard

People

Description

Hovering over a summary in the commit description table now shows a
tooltip with copyable text if the commit's description is longer than
he summary. Moving the mouse off of the tooltip causes it to disappear.

Testing Done

Verified that the description was correctly shown.

The tooltip's text align's pixel-perfectly with the table's text in
Chrome, Firefox, and Safari on OS X.

Issues

Description	From	Last Updated
Blank line after this.	david	May 4, 2015, 12:19 p.m.
Undo this change.	david	May 4, 2015, 12:19 p.m.
If you're using .text(), you shouldn't also need to escape.	david	May 4, 2015, 12:19 p.m.
These should go up below background-color. Also, no blank lines between the rules (as it gets harder to differentiate rules …	chipx86	May 6, 2015, 2:02 p.m.
Should be .box-shadow(...).	chipx86	May 6, 2015, 2:02 p.m.
We have a new .transition for this that does all the vendor-specific prefixes (though you'll need to pass duration as …	chipx86	May 6, 2015, 1:56 p.m.
This will put the cid key in diffCommit.attributes, which probably isn't what you want. You can instead put {} as …	chipx86	May 6, 2015, 1:59 p.m.
_.noop is built for this purpose. Not a big difference, except that it's one less function to create.	chipx86	May 6, 2015, 1:59 p.m.
Where does the 9 come from?	chipx86	May 13, 2015, 6:35 p.m.
Can you put the function body on its own line?	chipx86	May 6, 2015, 2 p.m.
This doesn't seem like a useful rule :)	david	May 15, 2015, 11:44 p.m.

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
Show all issues
```
Blank line after this.
```
reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 1)
Show all issues
```
Undo this change.
```
reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 1)
Show all issues
```
If you're using .text(), you shouldn't also need to escape.
```
1. brennie April 28, 2015, 2:57 p.m.
  Oops that should be .html().

Commit:

-	833ee3015400cf875af5bda8338892ddd8213a49
+	e85d8ce158d8289ffd214b8816b8c9dd26f1e33b

Diff:

Revision 2 (+74 -2)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)

Show all issues

These should go up below background-color. Also, no blank lines between the rules (as it gets harder to differentiate rules and nested styles).

Also, should use .border-radius(...).

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)
Show all issues
```
Should be .box-shadow(...).
```

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 2)

Show all issues

We have a new .transition for this that does all the vendor-specific prefixes (though you'll need to pass duration as part of the value).

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)

Show all issues

This will put the cid key in diffCommit.attributes, which probably isn't what you want. You can instead put {} as the first value to _.extend(), or use _.defaults. I think the latter is probably the best way to go.
I also think, for future expansion, that this should be more like:

tr = this._itemTemplate(_.defaults(
    {
        cid: diffCommit.cid
    },
    diffCommit.attributes));

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)

Show all issues

_.noop is built for this purpose. Not a big difference, except that it's one less function to create.

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)

Show all issues

Where does the 9 come from?

brennie May 6, 2015, 1:15 p.m.

The 9 comes from me trying to figure out how to make the tooltip pixel perfect. It isn't and I'm not sure how to get it there.

chipx86 May 7, 2015, 1:52 p.m.

Okay. Usually, the problem has to do with the padding of the parent, or border sizes if not using .box-sizing(border-box).

Assuming it's padding, you can use $myElement.getExtents('p', 'l') and .getExtents('p', 't') to get the left and top padding
of the element. You can also stick b and m in there if you need to calculate borders and margins, respectively.

brennie May 7, 2015, 1:59 p.m.
```
Alright, I'll give this a shot.
```

brennie May 7, 2015, 4:07 p.m.

So its quite close to being pixel perfect, but its off by just a bit now. I'm not sure how to get it any closer.

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 2)
Show all issues
```
Can you put the function body on its own line?
```

Commit:

-	e85d8ce158d8289ffd214b8816b8c9dd26f1e33b
+	003ee6747e6f5dc39a32eedcdb32e1119e1b22fa

Diff:

Revision 3 (+494 -212)

Show changes

	reviewboard/datagrids/columns.py
	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/mixins/style.less
	reviewboard/static/rb/css/pages/admin.less
	reviewboard/static/rb/css/pages/base.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/css/pages/newReviewRequest.less
	13 more

Tool: Pyflakes
Processed Files:
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/static/rb/js/newReviewRequest/views/repositoryView.js
    reviewboard/templates/datagrids/datagrid.html
    reviewboard/static/rb/js/newReviewRequest/views/newReviewRequestView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/static/rb/css/pages/newReviewRequest.less
    reviewboard/static/rb/js/views/uploadDiffView.js
    reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/css/ui/sidebars.less
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/datagrids.less
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/new_review_request.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/newReviewRequest/views/repositorySelectionView.js
    reviewboard/static/rb/css/mixins/style.less



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/datagrids/columns.py
    reviewboard/reviews/templatetags/reviewtags.py

Ignored Files:
    reviewboard/static/rb/js/newReviewRequest/views/repositoryView.js
    reviewboard/templates/datagrids/datagrid.html
    reviewboard/static/rb/js/newReviewRequest/views/newReviewRequestView.js
    reviewboard/templates/reviews/review_reply.html
    reviewboard/static/rb/css/pages/newReviewRequest.less
    reviewboard/static/rb/js/views/uploadDiffView.js
    reviewboard/static/rb/js/newReviewRequest/views/postCommitView.js
    reviewboard/static/rb/css/pages/diffviewer.less
    reviewboard/static/rb/css/ui/sidebars.less
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/base.less
    reviewboard/static/rb/css/ui/datagrids.less
    reviewboard/static/rb/css/pages/admin.less
    reviewboard/static/rb/js/newReviewRequest/views/commitView.js
    reviewboard/static/rb/css/pages/reviews.less
    reviewboard/templates/reviews/new_review_request.html
    reviewboard/static/rb/css/defs.less
    reviewboard/static/rb/js/newReviewRequest/views/repositorySelectionView.js
    reviewboard/static/rb/css/mixins/style.less

Commit:

-	003ee6747e6f5dc39a32eedcdb32e1119e1b22fa
+	a35fe64fcdedcb3df6be92caae76cc2a14164a48

Diff:

Revision 4 (+84 -2)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Change Summary:

Positioning adjustments. Renders perfect on Fx, but is off to top and right by 1px in chrome.

Commit:

-	a35fe64fcdedcb3df6be92caae76cc2a14164a48
+	7380c0781cda65c60d760882abfa5dedb3e1c259

Diff:

Revision 5 (+83 -2)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Change Summary:

Now pixel perfect in Chrome, Fx, and Safari on OSX.

Testing Done:

		Verified that the description was correctly shown.

~		TODO: Cannot seem to get this pixel perfect.
	~	The tooltip's text align's pixel-perfectly with the table's text in
	+	Chrome, Firefox, and Safari on OS X.

Commit:

-	7380c0781cda65c60d760882abfa5dedb3e1c259
+	afaa8208efdff40b406afb2225ef6fb2d3822a00

Diff:

Revision 6 (+87 -3)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js (Diff revision 6)

So I've managed to replace one magic number (9) with another (2), but this time it aligns pixel perfectly on all of my browsers.

I'm not sure if there is a better way to calculate this without it being a gigantic pain.

Change Summary:

Remove empty LESS rule.

Commit:

-	afaa8208efdff40b406afb2225ef6fb2d3822a00
+	94609f3b3c1c3089bb65c09edd9f181c61091d16

Diff:

Revision 7 (+87 -3)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Ship it!

reviewboard/static/rb/css/pages/diffviewer.less (Diff revision 7)

Show all issues

This doesn't seem like a useful rule :)

brennie May 15, 2015, 11:41 p.m.

Meant to fix that and uploaded the same commit instead! 1am coding is best coding.

Change Summary:

Fix David's issue. Refactor.

Commit:

-	94609f3b3c1c3089bb65c09edd9f181c61091d16
+	fe4fb6d3945a4e507118915449e7730f9e37da77

Diff:

Revision 8 (+79 -2)

Show changes

	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/diffviewer/views/diffCommitIndexView.js
    reviewboard/static/rb/css/pages/diffviewer.less

Status: Closed (submitted)

Change Summary:

Pushed to dvcs (a220c2d)