Ref T4714, fixing tooltips not appearing in editor when in fullscreen mode
Details
Details
- Reviewers
epriestley chad - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T4714: Tooltips do not appear in when editor is in fullscreen mode
- Commits
- Restricted Diffusion Commit
rPcbfa99174eb7: Fixing tooltips not appearing in fullscreen editor
Create paste, verify tooltips appear in comment text-editing bar, make comment box fullscreen, verify tooltips still appear.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- tooltip
- Lint
Lint Passed - Unit
No Test Coverage
Event Timeline
Comment Actions
Looks mostly good, but see some inlines.
| webroot/rsrc/css/core/z-index.css | ||
|---|---|---|
| 127–128 | 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. | |
| 86 | (Leftover debugging code.) | |
| 97 | 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. | |
| 100 | This will always be true unless a.y == s.y and a.x == s.x, so I think something is off here. | |
Comment Actions
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. | |
| 103 | By convention, prefer to name local variables in lowercase_with_underscores. See the style guide. | |