WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#29085 closed enhancement (fixed)

Media Grid: Remove padding and margins around left and right sides of grid

Reported by: melchoyce Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch commit dev-reviewed
Focuses: ui, administration Cc:
PR Number:

Description

The indentation on the left and right of the grid looks weird:

https://i.cloudup.com/YIk6ON7g7y.png

ryelle is working on a patch.

Attachments (5)

29085.diff (2.4 KB) - added by ryelle 5 years ago.
29085.2.diff (554 bytes) - added by ocean90 5 years ago.
29085.patch (2.0 KB) - added by iseulde 5 years ago.
29085.2.patch (2.0 KB) - added by iseulde 5 years ago.
29085.3.diff (2.0 KB) - added by helen 5 years ago.

Download all attachments as: .zip

Change History (19)

#1 @melchoyce
5 years ago

  • Focuses ui administration added
  • Milestone changed from Awaiting Review to 4.0
  • Type changed from defect (bug) to enhancement
  • Version set to trunk

@ryelle
5 years ago

#2 @ryelle
5 years ago

29085.diff removes the margins on the sides of the attachment UL and the first attachment per-column (the last attachment needs the padding so the checkbox isn't cut off).

#3 follow-up: @afercia
5 years ago

hi, I agree it would look really nicer without those margins but there are a couple of issues :)
About the first one, probably you don't notice anything weird because you're on a platform that uses "overlay scrollbars" e.g. OS X Lion+ / iOS / Windows 8 / Windows Phone / Android / Ubuntu Unity... but it's visible on other platforms for example in Windows 7- where scrollbars do occupy space.
For me, the attachments UL computed width in Chrome console is 1461px but this.$el.width() returns 1478 I guess that's because Chrome reserves space for the vertical scrollbar even if overflow is "auto". This issue has been there even before your patch but now is really noticeable: data.columns is 9 but I have space for just 8 columns, so the nth-of-type thing is really not working, see first screenshot:
http://i.imgur.com/xdqNVIt.jpg

Second issue: I agree the "indentation" is ugly and I did try to remove that margins too but unfortunately, focus style and "selected" style both use box-shadows that will be cut off on a side, see second screenshot:
http://i.imgur.com/lEim7nh.png

Please notice that at least 3 ancestor elements have an overflow value other than "visible" and I'm not sure they can be removed so easily.

#4 in reply to: ↑ 3 @afercia
5 years ago

Replying to afercia:

Please notice that at least 3 ancestor elements have an overflow value other than "visible" and I'm not sure they can be removed so easily.

gosh... my English :D of course I didn't mean to remove the elements but to change the overflow value.

#5 follow-up: @ocean90
5 years ago

#27423 will probably remove resizing via JS.

#6 in reply to: ↑ 5 @afercia
5 years ago

Replying to ocean90:

#27423 will probably remove resizing via JS.

that would be great.
However I was wrong, it's not just Chrome and it has nothing to do with overflow "auto". The main issue here is about timing. That's because this.$el.width() returns a value *before* the vertical scrollbar appears.
Say you have enough attachments to have a vertical scrollbar in your browser. Reload the page, and it will be empty for a while, waiting for attachments to appear. The duration of this "lag" is totally unpredictable, it will depend on so many factors: network speed, server speed, your CPU/GPU etc. However, this.$el.width() will return a value now, while the page is "empty" and before the scrollbar appears.
In my case, it returns 1462. Then, attachments appear and finally I have a vertical scrollbar, so the actual available width is 1445 (1462 - the scrollbar width). All the calculations (columns, edge) will still use 1462 while the real available width is 1445.

@ocean90
5 years ago

#7 @ocean90
5 years ago

  • Keywords has-patch added

29085.2.diff just reduces the padding by 6px, see [29376]. We can't use 0 because the checkboxes will be cut off then.

#8 @wonderboymusic
5 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 29425:

Media Grid: reduce the padding for .attachments.

Props ocean90, ryelle.
Fixes #29085.

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.


5 years ago

#10 @helen
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Caused negative effects on the media modal - avryl dropped a screenshot and a patch on #29483 before I dug out this ticket.

@iseulde
5 years ago

@iseulde
5 years ago

#11 @iseulde
5 years ago

Added more specific values for column widths so there's not a lot more space on the right side for the media grid. Left the 2px padding for the media grid, removed the padding for the media "selection" bar in the modal (was 2, should be 0) and added 8px padding for the media browser in the modal.

#12 @helen
5 years ago

  • Keywords commit added

Looks good.

@helen
5 years ago

#13 @helen
5 years ago

  • Keywords dev-reviewed added

#14 @helen
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 29687:

Media: Better padding for attachment items.

props avryl.
fixes #29085.

Note: See TracTickets for help on using tickets.