WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 9 months ago

#31029 assigned defect (bug)

Allow 0 columns in gallery settings

Reported by: afercia Owned by: rhurling
Milestone: Future Release 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 2 years ago.
Proposed patch
31029.1.patch (1.3 KB) - added by rhurling 23 months ago.
Updated previous patch with inline filter docs
31029.2.patch (1.3 KB) - added by rhurling 23 months ago.
Rephrasing of inline filter docs param
31029.3.patch (1.3 KB) - added by DrewAPicture 23 months ago.
Single hook approach

Download all attachments as: .zip

Change History (23)

#1 follow-up: @kraftner
2 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
2 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
2 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
2 years ago

I would vote filter too.

#5 @wonderboymusic
2 years ago

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

@GunGeekATX
2 years ago

Proposed patch

#6 @GunGeekATX
2 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
2 years ago

Tested, filters work great for me.

#8 @Stagger Lee
2 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
2 years ago

@StaggerLee The changes to core are needed as well.

#10 in reply to: ↑ 9 @Stagger Lee
2 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
23 months ago

Updated previous patch with inline filter docs

#11 @rhurling
23 months 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
23 months ago

Rephrasing of inline filter docs param

@DrewAPicture
23 months ago

Single hook approach

#12 @DrewAPicture
23 months 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
22 months ago

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

#14 @a4jp.com
16 months ago

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

#15 @a4jp.com
12 months ago

Will this be in WordPress 4.5?

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

#16 @kraftner
9 months 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
9 months 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 9 months ago by a4jp.com (previous) (diff)

#18 @kraftner
9 months 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
9 months 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.

Note: See TracTickets for help on using tickets.