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

Quick Tags, Quick Reblog: Fix popup horizontal cutoff #1536

Merged

Conversation

marcustyphoon
Copy link
Collaborator

@marcustyphoon marcustyphoon commented Jul 6, 2024

Description

Right now, it's possible for Quick Tags/Quick Reblog popups to extend past the left or right of their post elements (fundamentally fine) and be clipped off by the edge of the viewport or post column (not fine) in various circumstances. This fixes this by:

  • Removing the special "popup is on the left of the button" position entirely.
  • Adding logic that finds the closest ancestor element to the popup which would horizontally clip content, whatever it is, and shifts the popup's horizontal position to the left if necessary to keep the popup within that element's bounding box. (not implemented: shifting it right if it's too far to the left)

A simpler option, of course, would be to keep the popup horizontally within the post (const preventOverflowTarget = target.closest(postSelector)). When the viewport is wide enough not to need this I think it feels a bit more natural to leave it centered, but the difference is very small.

Another option, essentially the opposite, would be to target the viewport (adjusting position only on phone screens) and properly float the popups above e.g. the Patio column edges. I'm not sure if there's an elegant way to do this (HTMLDialogElement's showModal(), maybe?)

Incidental changes:

  • Unifies the popup placement code between Quick Tags and Quick Reblog.
  • Adjusts the Quick Reblog element DOM position back to where it used to be; closest('div') apparently now matches a "root" div class="v2JIl" element in <990px viewports that I don't think existed when we wrote the 12px override thingy.

Resolves #1369. Fixes mobile-device issue reported in the New XKit Discord that hasn't been made into an issue yet.

Testing steps

todo: write full list of tests performed. (patio narrow column, peepr wide, peepr narrow, embedded blog view, tablet/mobile layout, narrow phone layout via browser zoom /// top/bottom positions)

@marcustyphoon
Copy link
Collaborator Author

  • Should these positioning styles become classname and data attribute based, and live in one of our static stylesheets? Probably. I ran into a whole mess in quick tags search mvp #1528 trying to refactor the popups because the current logic overwrites the classname property and thus doesn't let you use classes for anything else

src/utils/interface.js Outdated Show resolved Hide resolved
@AprilSylph AprilSylph self-requested a review July 7, 2024 12:13
@AprilSylph AprilSylph linked an issue Jul 15, 2024 that may be closed by this pull request
@marcustyphoon marcustyphoon linked an issue Jul 17, 2024 that may be closed by this pull request
Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. Tested on 320px-wide viewport, edge of screen is avoided; tested on narrow (1000px-wide) desktop Peepr, edge of container is avoided! 🎉 I'll just trust your screenshot that the same logic works on Patio too...

@marcustyphoon marcustyphoon merged commit bc4c315 into AprilSylph:master Aug 18, 2024
2 checks passed
@marcustyphoon marcustyphoon deleted the prevent-horizontal-overflow branch August 18, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants