kaosko
commented
5 years ago Continued investigating the current popup placement code and looks like it already tries to avoid clipping. However, I think it's both needlessly complicated and ineffective. I don't think calculate-optimal-position, popover-clipping and calc-element-midpoint are needed, if calc-pop-offset would consider the bounds, taking into account both the anchor point and size of the panel, something like:
(let [rect (get-client-rect node) ...] (case arrow-pos :right (max (+ 20 position-offset) (:left rect)) :left (min (if p-width (- (- p-width 25) position-offset) p-width) (- (+ (:left rect) (:width rect))
Furthermore, I typically nowadays just store a ref in an atom, then use it when needed. The benefit is that you really don't need a form-3 component just to get a ref. Also, the atoms storing the component props shouldn't be r/atoms to avoid infinite loops and overall the code looks like perhaps its written a long time ago. So anyway, with all of these, it looks more like some amount of refactoring rather than a patch on top of everything, but I really would like to avoid adding complexity. Let me know if you'd be interested in taking a pull request for that. Right now I've only done the minimum amount needed to make the forked component work for my own use.
Popover position doesn't correctly consider the size of the screen, this is very visible on small screens (i.e. mobile). (popover-clipping) seems to work but (calc-pop-offset) just takes the popover's size as the offset, which I guess makes some sense as it's all tied to the arrow placement and the content's relative position to it. Position calculation seems complex as is, but I'd prefer a simpler positional keyword like :below (which could adjust to -left -center or -right) or even :automatic, to let the component pick a position that is guaranteed to render on screen.