use visualViewportStyle for inactivity modal#13200
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the shared Modal component to keep the kiosk inactivity redirect modal centred within the user’s current visual viewport (including when the page is zoomed/panned), using the VisualViewport API as requested in #13196.
Changes:
- Adds VisualViewport-driven positioning logic for
modalStyle="inactivity"when the modal is active. - Applies computed inline styles to the modal window to keep it visible and appropriately sized during zoom/pan.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| const style: React.CSSProperties = { | ||
| position: 'fixed', | ||
| top: `${vv.offsetTop + vv.height / 2}px`, | ||
| left: `${vv.offsetLeft + vv.width / 2}px`, | ||
| right: 'auto', | ||
| bottom: 'auto', | ||
| transform: 'translate(-50%, -50%)', | ||
| maxWidth: `${Math.min(550, vv.width * 0.9)}px`, | ||
| maxHeight: `${vv.height * 0.9}px`, | ||
| width: '80%', | ||
| }; |
There was a problem hiding this comment.
This is me just coming to this but I was wondering, if it's just for inactivity, could it be added here? https://github.com/wellcomecollection/wellcomecollection.org/blob/main/common/views/components/Modal/Modal.styles.tsx#L154 (I guess we'd have to pass the value of vv...)
There was a problem hiding this comment.
yes, thank you, I had meant to move them there then forgot
There was a problem hiding this comment.
actually, I think the runtime calculated values is what stopped me. I think it makes sense here
There was a problem hiding this comment.
OK I think it's just the style happening in various places that might be confusing to debug in the future but that's quite a niche spot. Can we leave a comment to explain to future us?
rcantin-w
left a comment
rcantin-w
left a comment
There was a problem hiding this comment.
Works well, the simulator shows me the modal as full screen when zoomed though, I couldn't replicate a broken behaviour.
A few suggestions to performance perhaps (less maths/listeners if not required)
| const style: React.CSSProperties = { | ||
| position: 'fixed', | ||
| top: `${vv.offsetTop + vv.height / 2}px`, | ||
| left: `${vv.offsetLeft + vv.width / 2}px`, | ||
| right: 'auto', | ||
| bottom: 'auto', | ||
| transform: 'translate(-50%, -50%)', | ||
| maxWidth: `${Math.min(550, vv.width * 0.9)}px`, | ||
| maxHeight: `${vv.height * 0.9}px`, | ||
| width: '80%', | ||
| }; |
There was a problem hiding this comment.
OK I think it's just the style happening in various places that might be confusing to debug in the future but that's quite a niche spot. Can we leave a comment to explain to future us?
| return; | ||
| } | ||
|
|
||
| if (typeof window === 'undefined' || !window.visualViewport) { |
There was a problem hiding this comment.
Could we also return if vv.scale !== 1? https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport/scale
And actually I wonder if the listeners are worth it since this is just from when the Modal displays, right?
| import { FocusTrap } from 'focus-trap-react'; | ||
| import { | ||
| FunctionComponent, | ||
| MutableRefObject, |
There was a problem hiding this comment.
This is unrelated to this PR so feel free to ignore, but running it locally shows me this is deprecated and we should use RefObject (which we also import). If the switch is easy it could be nice to move it over!
Also I had a linting change on save, but maybe I'm running a newer version.
What does this change?
Ensures the redirect modal is visible regardless of the zoom state
Previously the modal could be totally offscreen or clipped.
Before:
After:
How to test
Have we considered potential risks?
it affects other modals? - this is gated by the 'inactivity' model type so the affect is isolated