Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define the lightbox size in the image size settings #545

Closed
leofeyer opened this issue Jul 3, 2019 · 17 comments

Comments

Projects
None yet
7 participants
@leofeyer
Copy link
Member

commented Jul 3, 2019

In #529, we have added the tl_layout.lightboxSize field, which allows us to define an image size for lightbox images. As briefly discussed in Slack, I think that the field does not belong to the page layout and might be better placed in the content element or module where it is used.

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Hm, but then an editor would need to do define that for every content element, which, I am assuming, will largely be always the same setting.

Sure, you could define a default via your own DCA configuration. But it seems rather cumbersome.

On that note: we frequently have the use case, that consumers always want a default image size for certain section or elements. May be Contao could integrate a solution for that somehow? ;)

@Toflar

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I don't think that really makes sense. It can though but still it should work with some fallback implementation which would be fairly easy to integrate: Provide the option in the content element and if that's not there, fall back to the layout 🤷‍♂

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

Hm, but then an editor would need to do define that for every content element

Just like they need to define the image size for every content element. I fail to see a difference here.

@fritzmg

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I know - most of our customers never need that though and thus it is always a default image size (or a dynamic one that we programmed). Having the lightbox image size setting for every content element is overkill in my opinion and just adds to this nuisance.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

You realize that the field is only visible if the user has checked the "Full-size view" checkbox?

@asaage

This comment has been minimized.

Copy link

commented Jul 3, 2019

If at all It should be in Theme->image-sizes i think.
Maybe we could check for an entry named lightboxSize and use this otherwise fallback to Full-size - so we don't need a select there.

@m-vo

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I think people that want to have more finegrained control over a 'per theme' setting are misusing lightboxes already (UX). 😉

I know - most of our customers never need that though and thus it is always a default image size (or a dynamic one that we programmed).

I second that. That's why I'm using lots of custom elements instead of the system ones so that the element itself can decide which image sizes to pick.

We could also follow such a route (I know this is a way bigger scope than this issue but think about it):

  • allow to configure everything in the CEs
  • allow defining different 'base configurations' of a CE together with a new name (e.g. in the theme) where some of the fields are already filled out
  • these preconfigured CEs then (also) appear in the list of CEs

Then you could for example have the CEs 'Text with image on the left', 'Galery with huge lightbox', 'Small note'.

@ausi

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I don’t see the use case for setting the lightbox size in a content element. In 99.99% of cases this is not necessary and the 0.01% can easily extend the content element for their own needs. It is also not possible in the content element settings to select which lightbox should get used (colorbox or mediabox).

The look (CSS) and feel (JavaScript) of the lightbox is defined in the page layout and I think this is also the best place to define the size of the lightbox.

@Toflar

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

I'm with @ausi here.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

It is also not possible in the content element settings to select which lightbox should get used (colorbox or mediabox).

That's true, the lightbox itself is defined in the page layout. But you can use this one lightbox in many different content elements and define multiple image sizes for the preview images. So why can we only have one fixed size for the full-size images? Makes no sense to me.

@ausi

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

So why can we only have one fixed size for the full-size images?

Because they should all be “full-size”. Just as they currently are in Contao 4.7.

The lightbox image size would mostly be used to set a maximum width/height, like [2048, 2048, 'box'] and to configure resize options like the new skipIfDimensionsMatch flag.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

There are several legit use cases for using different full-size sizes:

  • I want the full-size images of gallery 1 to be 1280x720 (crop) and the ones of gallery 2 to be 1280x1280 (fit the box).
  • I have a gallery that is supposed to show only the important parts of its images (thumbnails and full-size images). My other galleries shall render normally.
@asaage

This comment has been minimized.

Copy link

commented Jul 3, 2019

different full-size sizes

this should definitely be called different then.
Having it on the content-element-level could be useful.

@ausi

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

There are several legit use cases for using different full-size sizes

I never came across any of those use cases in my lifetime so far, and I don’t remember any feature request for such a functionality.

I don’t say that this will never be needed in a single project, but I don’t want to bloat the settings of every content element that has the “Full-size” checkbox, just to support a use case for 0.01% of users.

different full-size sizes

this should definitely be called different then.

It would probably be called “Lightbox image size” as you can see in the screenshot above.

@aschempp

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I don't like Lightbox image size. It actually is the "full size" or kinda "maximum size". If I don't enable a lightbox, the respective image will be shown in a (new) browser tab.

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 4, 2019

As discussed in Mumble on July 4th, we want to keep the lightbox settings in the page layout. Additionally, we want to move the tl_theme.defaultImageDensities field to tl_layout so we have the default image settings in the same place.

@leofeyer leofeyer added defect and removed up for discussion labels Jul 4, 2019

@leofeyer leofeyer added this to the 4.8 milestone Jul 4, 2019

@leofeyer leofeyer self-assigned this Jul 5, 2019

leofeyer added a commit that referenced this issue Jul 5, 2019

@leofeyer

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Fixed in ed1d67e.

@leofeyer leofeyer closed this Jul 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.