Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popover may easily render off-screen on small screens #191

Open
kaosko opened this issue Jan 10, 2019 · 1 comment
Open

Popover may easily render off-screen on small screens #191

kaosko opened this issue Jan 10, 2019 · 1 comment
Assignees

Comments

@kaosko
Copy link

kaosko commented Jan 10, 2019

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.

@kaosko
Copy link
Author

kaosko commented Jan 11, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants