Only set absolute positioning in modal - #12797
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
|
||
| .media-frame .media-toolbar-secondary .spinner { | ||
| .media-modal .media-frame .media-toolbar-secondary .spinner { | ||
| top: calc( 50% - 8px ); |
There was a problem hiding this comment.
At this media query, it still looks misplaced:
There was a problem hiding this comment.
What would you suggest as an alternative? The spinner applies to all filters. So aligning it to one or the other feels wrong; this alignment is vertically centered.
There was a problem hiding this comment.
Another alternative would be to set it at calc( 50% - 10px ), and move the position: relative to .media-toolbar instead of .media-toolbar-secondary. That's a relative major change, however, and I'm not sure it's a good idea - there could be unanticipated side effects.
There was a problem hiding this comment.
Tested it, and yes, there are definitely undesirable side effects...
afercia
left a comment
afercia
left a comment
There was a problem hiding this comment.
I left a couple points to address..
Also, in the media dialog 'details' panel on the right, a text Saved. appears together with the spinner. The absolut epositioning overlays the spinner on top of the text.
In the screenshot below, how it looks on trunk compared to WP 4.8. Note that this specific spinner appears to have regressed a few times on various past releases so that I traveled back in time till WP 4.8 intentionally to have a confirmation that it was as I remembered:
|
I think it's reasonable to adjust the specific barrier issues for spinners in this ticket, but not change the existing positioning significantly. There's no clear "winning" position for the spinner on mobile - ideally, it would either be in a clear global position, or always be associated with whatever field triggered it. Those are both more significant changes, however. |
This appears to fix the overlap, while still retaining the fix from #27914
|
Attachment details spinner after change: 
|
Two positioning issues: on desktop, the active spinner appeared off screen, generating a scrollbar in the media toolbar. In the attachment details modal, the spinner overlapped with the `Saved` confirmation. On desktop, limit some positioning assignments to only apply with the media modal. In the attachment details, apply `display: flex` to prevent overlapping. Developed in #12797 Props afercia, rcorrales, joedolson. Fixes #65778. git-svn-id: https://develop.svn.wordpress.org/trunk@63029 602fd350-edb4-49c9-b593-d223f7449a82
Two positioning issues: on desktop, the active spinner appeared off screen, generating a scrollbar in the media toolbar. In the attachment details modal, the spinner overlapped with the `Saved` confirmation. On desktop, limit some positioning assignments to only apply with the media modal. In the attachment details, apply `display: flex` to prevent overlapping. Developed in WordPress/wordpress-develop#12797 Props afercia, rcorrales, joedolson. Fixes #65778. Built from https://develop.svn.wordpress.org/trunk@63029 git-svn-id: http://core.svn.wordpress.org/trunk@62248 1a063a9b-81f0-0310-95a4-ce76da25c4cd
2 participants
Limit the spinner absolute positioning to only take effect in the media modal.
Trac ticket: https://core.trac.wordpress.org/ticket/65778
Use of AI Tools
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.