WIP Display popover intelligently - #2599
Conversation
| const top = targetTop - height; | ||
|
|
||
| return { left, top }; | ||
| return { left, top, dir: "up" }; |
There was a problem hiding this comment.
Nice.
We might also want to do left/right here in a bit
| let top = targetBottom; | ||
| return { left, top }; | ||
| if (top + height > window.innerHeight) { | ||
| console.log("Going out of bounds"); |
Codecov Report
@@ Coverage Diff @@
## master #2599 +/- ##
==========================================
- Coverage 58.55% 57.76% -0.79%
==========================================
Files 51 51
Lines 1959 1996 +37
Branches 398 401 +3
==========================================
+ Hits 1147 1153 +6
- Misses 812 843 +31
Continue to review full report at Codecov.
|
| const { top, left } = this.state; | ||
| const { top, left, dir } = this.state; | ||
|
|
||
| if (dir === "up") { |
There was a problem hiding this comment.
lets see if we can consolidate these two renders into one...
| if (dir === "up") { | ||
| return dom.div( | ||
| { | ||
| className: "popover popover-up", |
There was a problem hiding this comment.
we can use the classnames helper to do classnames("popover", { up: dir =="up" })
There was a problem hiding this comment.
Didn't know about that. Yeah, we can use this.
| onMouseLeave, | ||
| style: { top, left } | ||
| }, | ||
| children, |
There was a problem hiding this comment.
the ordering here is the hard one, but we can likely do something clever :)
try to reduce size of render functions
|
Hey @AnshulMalik I made some additional follow up tweaks
Let me know what you think and we can land this soon. Here's a GIF of how it looks now: Light ThemeDark ThemeI think it's looking pretty good! The only follow up item I see is that we will want to also adjust the horizontal (left/right ) orientation as well. But, lets do this in a different a new issue! |
|
Looks good to me. |
* Display popover intelligently * popover ui improvements try to reduce size of render functions
Associated Issue: #2565
Summary of Changes
Screenshots