Make WordPress Core

Opened 10 years ago

Last modified 2 years ago

#31029 assigned defect (bug)

Allow 0 columns in gallery settings

Reported by: afercia's profile afercia Owned by: rhurling's profile rhurling
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: Media Keywords: good-first-bug has-patch
Focuses: ui, javascript Cc:

Description

Gallery columns "0" is a valid setting and correctly outputs a proper CSS class
class="gallery galleryid-43 gallery-columns-0 gallery-size-thumbnail">

Using that CSS class, as a theme author, I could easily build, for example, a grid layout with items displayed inline (or whatever layout other than "columns"), overriding just a very few CSS rules.

I could set columns="0" manually, however, each time I'd edit the gallery settings through the media views UI, columns="0" would be removed and it would fallback to the default 3 columns.

https://cldup.com/7zoOccOC1H.png

I would propose to add and allow a "none" columns setting, with value "0".

P.S. just noticed: same if I want to add a value higher than "9". Setting an arbitrary limit to "9" is an assumption that, as developers, we shouldn't do. What if I want, say, 10 images per row?

Attachments (4)

31029.patch (897 bytes) - added by GunGeekATX 10 years ago.
Proposed patch
31029.1.patch (1.3 KB) - added by rhurling 10 years ago.
Updated previous patch with inline filter docs
31029.2.patch (1.3 KB) - added by rhurling 10 years ago.
Rephrasing of inline filter docs param
31029.3.patch (1.3 KB) - added by DrewAPicture 10 years ago.
Single hook approach

Download all attachments as: .zip

Change History (25)

#1 follow-up: @kraftner
10 years ago

In theory I think this is a great idea...

... if there weren't a lot of themes that will probably not handle that situation well.

#2 in reply to: ↑ 1 @afercia
10 years ago

Replying to kraftner:

In theory I think this is a great idea...

... if there weren't a lot of themes that will probably not handle that situation well.

The worst possible scenario is that the gallery will look ugly and editors will soon revert that setting to a 1-9 value.
Having those values generated by a loop in a template (see media-template.php) without, as far as I can tell, any chance to filter them, doesn't look right to me.

Responsiveness is important, and with the advent of CSS Flexbox it would probably be a nice move to allow a "no columns" value. About the upper limit, I'm open to different thoughts :)

#3 @kraftner
10 years ago

Adding a filter sounds better to me than changing the default in core. Because then you have to intentionally change it if your theme supports it.

#4 @valendesigns
10 years ago

I would vote filter too.

#5 @wonderboymusic
10 years ago

  • Keywords needs-patch needs-docs good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

@GunGeekATX
10 years ago

Proposed patch

#6 @GunGeekATX
10 years ago

  • Keywords has-patch added; needs-patch removed

Added filters for min and max values, as well as a filter to customize the name. Example:

add_filter( 'media_gallery_columns_min', 'ticket_31029_media_gallery_columns_min' );
add_filter( 'media_gallery_columns_max', 'ticket_31029_media_gallery_columns_max' );
add_filter( 'media_gallery_columns_name', 'ticket_31029_media_gallery_columns_name' );


function ticket_31029_media_gallery_columns_min( $columns ) {
	return 0;
}

function ticket_31029_media_gallery_columns_max( $columns ) {
	return 20;
}

function ticket_31029_media_gallery_columns_name( $column ) {
	return $column === 0 ? __( 'None' ) : $column;
}

https://i.imgur.com/ka9i7va.png

#7 @sumobi
10 years ago

Tested, filters work great for me.

#8 @Stagger Lee
10 years ago

Do we neeed use filters together with core code change or as standalone ?
I tested on 2 websites, and cannot get it to work.

#9 follow-up: @kraftner
10 years ago

@StaggerLee The changes to core are needed as well.

#10 in reply to: ↑ 9 @Stagger Lee
10 years ago

Replying to kraftner:

@StaggerLee The changes to core are needed as well.

Thank you. I will wait tills it goes inside core.

@rhurling
10 years ago

Updated previous patch with inline filter docs

#11 @rhurling
10 years ago

I updated the patch with inline docs for 2 of the three filters, but I'm not sure where to add the inline docs for the third filter.
Any feedback on that (or the already added inline docs) would be great.

@rhurling
10 years ago

Rephrasing of inline filter docs param

@DrewAPicture
10 years ago

Single hook approach

#12 @DrewAPicture
10 years ago

  • Keywords needs-docs removed

I could advocate for one of three approaches here:

  1. Do nothing
  1. The "decisions not options" route, where we just modify the minimum to 0 columns, as the ticket description suggests

If we did this, we'd need to put in logic to ensure 0 would not be the default-selected item unless it was in the user settings.

  1. Add a single hook, a filter that allows specifying both the minimum and maximum number of columns at the same time. The hook on the option name would be skipped as it is out of scope for this ticket.

The single hook approach is done in 31029.3.patch

#13 @obenland
9 years ago

  • Owner set to rhurling
  • Status changed from new to assigned

#14 @a4jp.com
9 years ago

Wow. This looks great. Thank you for making this. I want to add responsive galleries by default to WordPress too.

#15 @a4jp.com
9 years ago

Will this be in WordPress 4.5?

It would be amazing having a gallery that auto adjusts finally.

#16 @kraftner
8 years ago

I think approach three is the best one.

What I am wondering is if we should check the sanity of the filtered values. E.g. that max>=min so it is impossible to create an endless loop. Or is this overcautious?

#17 @a4jp.com
8 years ago

It's great to hear any ideas here.

I think 0 should be written as auto and there should also be a width option for the image holder div that appears when you select auto (--- %/px/em/...). If the div isn't set to 100% align options (icons) would also be nice.

@kraftner maybe we could type in a number for pagination.
Number of images before a new page ---/auto

I have noticed one problem with pagination when I'm in Google Analytics though. It thinks each pagination is a new page so it says I have pages with the same content on them. It would be nice to be able to fix this problem somehow.

Last edited 8 years ago by a4jp.com (previous) (diff)

#18 @kraftner
8 years ago

@a4jp.com I think all the things you're mentioning are requests for new/other features that shouldn't be discussed here to keep this really only about the column count.

#19 @a4jp.com
8 years ago

Maybe I didn't understand what Afercia meant properly. If the column number is set to 0 that would mean there are no images showing at all, right? Does he mean turn off all gallery images? I initially thought he wanted auto column numbers which is something I have wanted in WordPress for years. Sorry if I thought his point was different.

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

Note: See TracTickets for help on using tickets.