Skip to content

Fixes #48 - Better handling of background images#141

Closed
mabolek wants to merge 6 commits intot3kit:masterfrom
mabolek:feature48
Closed

Fixes #48 - Better handling of background images#141
mabolek wants to merge 6 commits intot3kit:masterfrom
mabolek:feature48

Conversation

@mabolek
Copy link
Contributor

@mabolek mabolek commented Mar 21, 2017

No description provided.

Copy link
Contributor

@MattiasNilsson MattiasNilsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, just some smaller nitpicks! 👍

}

page.cssInline {
# Debug related output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented code.

@pixelmatseriks
Copy link
Member

It is a great feature to be able to have different images for different devices, but this commit will affect all GridElement CE:s (f.ex. Parallax), not just the advanced grid. We need to have a way to enable it per CE. Maybe there is a way to implement it using the new "cropVariant"?

@mabolek
Copy link
Contributor Author

mabolek commented May 3, 2017

@pixelmatseriks cropVariant support would probably be good in any case (I haven't looked too closely at it yet), but regarding the feature affecting other grid element CEs: Wouldn't it be good to have responsive images for parallax as well? Have I missed your point? 🙂

@pixelmatseriks
Copy link
Member

@mabolek Not all of the gridelements CE:s uses the image as a background image.

@mabolek
Copy link
Contributor Author

mabolek commented May 5, 2017

@pixelmatseriks Hmm... In that case, the current implementation has the same problem. All GridElements have a style="background-image: url('{f:uri.image(src: '{files.0.uid}', treatIdAsReference: 1)}')" if there's a file referenced in the content element. Only difference is that this pull request compresses the file so it's no larger than necessary.

@pixelmatseriks
Copy link
Member

@mabolek hmm, you are right. The Default layout always include a background-image if there's a file referenced in the content element, but the Empty layout doesn't. There is a condition in Collapsible, SimpleAccordion and Tab to use Default or Empty, but all of the other templates uses Default. The Parallax also includes file.0 if present in the template itself, but maybe it is a "fallback".

@mabolek
Copy link
Contributor Author

mabolek commented May 10, 2017

@pixelmatseriks I suggest we use the current implementation and open separate feature requests for 1) implementing cropVariant and 2) improving the way layouts work, so files are only included in CEs that need them.

@dmh
Copy link
Member

dmh commented May 17, 2017

#211

@dmh dmh closed this Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants