Page MenuHomePhabricator

Fixing tooltips not appearing in fullscreen editor
ClosedPublic

Authored by lpriestley on Apr 11 2014, 10:51 PM.

Details

Summary

Ref T4714, fixing tooltips not appearing in editor when in fullscreen mode

Test Plan

Create paste, verify tooltips appear in comment text-editing bar, make comment box fullscreen, verify tooltips still appear.

Diff Detail

Repository
rP Phabricator
Branch
tooltip
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

lpriestley retitled this revision from to Fixing tooltips not appearing in fullscreen editor.
lpriestley updated this object.
lpriestley edited the test plan for this revision. (Show Details)
lpriestley added a reviewer: epriestley.
epriestley edited edge metadata.

Looks mostly good, but see some inlines.

webroot/rsrc/css/core/z-index.css
128–130

We should move this to the bottom of the file so the tips remain in sorted order.

webroot/rsrc/js/core/ToolTip.js
51–52

Since a is a JX.Vector, you can simplify this slightly:

a.setPos(node);

Also, consider a more meaningful name for the variable a.

55

Since this method isn't meant to be called from outside of JX.Tooltip, preface the method name with an underscore: _getPosition

56–62

This looks like debugging code.

88

(Leftover debugging code.)

101

I don't think this is worth adjusting since no one will ever hit it (at least, we can wait for someone to complain), but since we're only looking at one position (the top left corner of the tip), this could give us the wrong result for some tips. Fore example, if if the thing being tipped is near the bottom of the screen and the tip has "S" alignment, the top left corner of the tip might be visible, but most of the tip text might not be.

If we wanted to be perfect here, we could check that all four corners of the tip are on screen.

104

This will always be true unless a.y == s.y and a.x == s.x, so I think something is off here.

This revision now requires changes to proceed.Apr 11 2014, 11:21 PM
lpriestley edited edge metadata.

Account for all four corners of tooltip when determing on/offscreen existence.

epriestley edited edge metadata.

Two tiny naming nitpicks, I'll just tweak those when I pull. Thanks!

webroot/rsrc/js/core/ToolTip.js
59–60

We should underscore-prefix these too.

107

By convention, prefer to name local variables in lowercase_with_underscores. See the style guide.

This revision is now accepted and ready to land.Apr 12 2014, 12:53 PM
epriestley updated this revision to Diff 20789.

Closed by commit rPcbfa99174eb7 (authored by @epriestley).