#30725 closed defect (bug) (fixed)
Add media modal - different select width
Reported by: | pavelevap | Owned by: | helen |
---|---|---|---|
Milestone: | 4.2 | Priority: | normal |
Severity: | minor | Version: | 4.1 |
Component: | Media | Keywords: | needs-patch |
Focuses: | ui | Cc: |
Description
When clicking "Add media", images are loading in modal window.
But there is different width when loading spinner appears.
When everything loaded, select field shrinks back and it is distracting.
Also, localized string is longer than English original and it is not visible the whole text.
See attached screenshot.
Attachments (11)
Change History (59)
#2
@
10 years ago
- Focuses ui added
Looks like it's visible each time the spinner is shown. After r30813 those selects have a percentage max-width value, when the spinner appears, it will increase the width of their container so they will "grow" and then "shrink" when the spinner disappears.
Also, a max-width is not good for languages other than English that may translate the option values with longer strings.
#3
follow-up:
↓ 6
@
10 years ago
- Milestone changed from Awaiting Review to 4.1.1
This is a pretty amusing bug.
max-width is OK, we don't really have a choice often to put a sane limit on things, but we're usually pretty good about giving translators at least 50% to 100% more space over what English requires.
#5
@
10 years ago
- Keywords has-patch added
I've added a patch that fixes this issue. There were a few ways to fix this, but I chose to absolute position the spinner and add container padding to the right. We could have replaced the display attribute with visibility to maintain the flow of elements as a second option. However, the pure CSS option seems like the best because the alternative would require JS modifications. Please test and let me know if you have any suggestions.
Cheers,
Derek
#6
in reply to:
↑ 3
@
10 years ago
Replying to nacin:
max-width is OK
I'd propose then to follow @valendesigns solution but using max-width on the select
elements only when needed, within a media query. Updated patch uses max-width: 1120px
as breakpoint, to be honest it's a bit arbitrary, please let me know if you have any suggestions about a better breakpoint. Tried also some CSS cleaning.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
10 years ago
#8
@
10 years ago
Patch looks good, just had to add one semi-esoteric rule so that options with long strings don't invisibly take up space. Committing.
#9
@
10 years ago
- Owner set to helen
- Resolution set to fixed
- Status changed from new to closed
In 31197:
#12
in reply to:
↑ 11
;
follow-up:
↓ 14
@
10 years ago
- Keywords needs-patch added; has-patch removed
Replying to afercia:
Please consider to remove or limit to modals that
width: 100%; /* prevents options from invisibly taking up horizontal space */
It wasn't in the original patch. We probably should have done more testing if the CSS was going to change. Helen, I know you have an inline comment, but can you go into more detail about what was happening to the UI that required adding that line?
#13
@
10 years ago
- Keywords has-patch added; needs-patch removed
After testing 31198 it's not immediately obvious what width: 100%
is meant to fix, because it doesn't change the UI for the affected modal and spinner jiggling. It probably shouldn't have ever made it into the commit. In any case, I've added patch 30725.3.diff that removes it and everything goes back to normal.
#14
in reply to:
↑ 12
@
10 years ago
Replying to valendesigns:
can you go into more detail about what was happening to the UI that required adding that line?
See attached image above. I'll look at this some more.
#15
@
10 years ago
The max-width on the select elements in the media library doesn't account for the button being there. Might need to rethink that approach for this instance (it's fine in the media modal).
With the original patch, we would get this:
#16
@
10 years ago
Sorry! That makes sense now. My bad. Here's an updated patch with better scoping.
#17
@
10 years ago
- Keywords needs-patch added; has-patch removed
Never mind. Patch is not complete. I didn't see your comments just now. The scoping works for modals but doesn't work for the media library. Back to the drawing board.
#18
@
10 years ago
However, If I remember correctly, the spinner was all the way to the right in mobile even before the patch.
#19
@
10 years ago
- Keywords has-patch added; needs-patch removed
I just did a double check and 4.1 has the spinner all the way right in mobile. So realistically, nothing changed with the spinner position and 30725.4.diff should be the correct course of action to fix long titles and properly scope the .media-modal
.
#21
@
10 years ago
We could then scope for .ie8
and set the width to auto
or something of that nature.
#22
@
10 years ago
Was thinking to switch to an inline formatting model, let browsers do their job, and don't think about this issue ever again :) using the text-align: justify technique could work in IE 7 too and, in the future, maybe using CSS flexbox.
About the spinner, what about to display it as an overlay above the UI controls? See examples below (backgrounds and opacity are there just for testing purposes while I was doing some in-browser editing). Would appreciate any feedback.
This ticket was mentioned in Slack in #core by afercia. View the logs.
10 years ago
#24
@
10 years ago
A relatively easy fix would be to keep the old CSS, make the spinner display: inline-block; visibility: hidden;
(or no display
at all) and toggle visibility from JS. In fact all spinners would work better if we adopt that everywhere.
#25
@
10 years ago
- Keywords needs-patch added; has-patch removed
I knew I was forgetting something - azaozz is right about the bigger picture, which we had discussed two years ago on #22839 :) Let's fix it for this instance in 4.1.1, and look to standardize across the board in 4.2 on that ticket.
#26
@
10 years ago
Would like to try to patch this but can't do without patching also #29897 please let me know how to proceed.
#27
@
10 years ago
- Keywords has-patch added; needs-patch removed
30725.2.diff should do it for this instance and reverts the previous commit - way simpler fix in the end to do it the right way. Please test.
#28
follow-up:
↓ 29
@
10 years ago
Tested, noticed some minor issues in responsive view and big issues in IE 8.
Please consider that percentage based calculation can't reserve enough space for the spinner when max-width kicks in. Say the max-width: 66% computes to 300px. The width taken by the two selects is:
1px + 47% + 2% + 1px + 47% + 2% = 296px
#29
in reply to:
↑ 28
;
follow-up:
↓ 30
@
10 years ago
Replying to afercia:
Tested, noticed some minor issues in responsive view and big issues in IE 8.
Is that caused by the patch? Is it new in 4.1?
#30
in reply to:
↑ 29
@
10 years ago
Replying to helen:
Is that caused by the patch? Is it new in 4.1?
Already existing in 4.1, except for the new spinner thing. Please notice the spinner inherits float: right
so it will behave like block
not like inline-block
. Maybe should be reset to float: none
? but still there wouldn't be enough space for the spinner in responsive view.
About IE 8, this could work:
.ie8 .media-toolbar-primary { width: 33%; } .ie8 .media-toolbar-secondary { width: 66%; } .ie8 .media-toolbar-primary #media-search-input { float: right; }
#31
@
10 years ago
Since we're looking at 4.1.1 here, what here needs to be fixed for a point release? Is this perhaps minor enough to do in 4.2 instead? I tend to think the latter at this point, especially if we've got more wide-reaching issues.
This ticket was mentioned in Slack in #core by dd32. View the logs.
10 years ago
This ticket was mentioned in Slack in #core by dd32. View the logs.
10 years ago
#34
@
10 years ago
- Keywords needs-patch added; has-patch removed
- Milestone changed from 4.1.1 to 4.2
I haven't seen other reports of the IE8 issue so it doesn't seem like an urgent fix, and the jiggling inputs are certainly also a bug that should be fixed, but it is minor and kind of funny. I'm going to revert the commit from the 4.1 branch and move this to 4.2, where we should fix it more holistically in terms of spinner visibility (#22839) and the percentage widths that don't account for things like the bulk select button or spinner.
#36
@
10 years ago
I should note that we can always move this back to 4.1.2 should one exist and the end fix be relatively self-contained.
#39
follow-up:
↓ 40
@
10 years ago
I have no idea what I'm doing in 30725.5.diff, but it brings back the spinner for me in wp-admin/upload.php
. It's currently missing.
#40
in reply to:
↑ 39
@
10 years ago
Replying to obenland:
It's a good start - we need to alter the relative width of the selects here, though, as the current percentages don't account for a spinner. Also needs an IE8 audit, I believe.
#41
@
10 years ago
See select_ie8.png for IE8 with 30725.5.diff applied.
This ticket was mentioned in Slack in #core by drew. View the logs.
10 years ago
#43
@
10 years ago
Far from being perfect, attached refreshed patch tries to fix the selects and the spinner position. Very, very, tricky:
- there's a mix of elements with fixed widths and percentage widths
- even using
calc()
we can't predict the width of some elements, for example the "Bulk select" button width will vary depending on the translation
Maybe the most sane things to do is to reserve additional space for longer translations, as much as possible, and accept the fact elements will flow in 2 or 3 lines on small screens.
There are still many edge cases, things may break. I'd consider this just an attempt to patch a layout that should really be rebuilt from scratch. We probably need to iterate on this. @helen please check carefully, any change and improvements more than welcome.
Note: url.js
needs some attention, the spinner isn't revealed when using "Insert from URL".
It worked without problems in 4.0.1 and date filter was not available here.