Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Add image sizes and the <picture> element #7296

Closed
wants to merge 63 commits into from

Conversation

ausi
Copy link
Member

@ausi ausi commented Sep 7, 2014

As discussed in #6932

Remaining to-dos:

  • Modify all core templates using images, to use the new picture template.
  • Add a user interface to select the important part of the image.
  • Create a beautiful image sizes icon.
  • Translate all messages, done for german and english.
  • Theme export and import (including transformation of IDs in imageSize columns)
  • Disable with and height input fields in the image size widget if an image size is selected
  • Remove all static methods in Image class, make it use object methods, see Toflars PRs Made Image class way more modular and object oriented madeyourday/contao-core#2 and First try of fixing tests madeyourday/contao-library#1
  • Fix breadcrumbs for tl_image_size_item
  • Merge SVG support changes (15201d0 and more)
  • Add important part support for SVG images

To discuss:

  • What is the right prefix for the picture template? block_ doesn’t seem right.
  • Should we make the picturefill polyfill optional? How?
  • Should we really remove the src attribute from the <img> element, as recommended by picturefill?
  • Issue PNG optimisation while resize #2426 wasn't solved correctly. The size of the PNG palette is taken from the source image, which is wrong. The number of colors of the generated image has to be used instead. Is my fix for this issue OK?

@ausi ausi mentioned this pull request Sep 7, 2014
@Toflar
Copy link
Member

Toflar commented Sep 8, 2014

Cool! Imho I'd add the following ToDo:

  • Remove all static methods in Image class, make it use object methods and keep ::get() as well as ::resize() for BC. Then number of method parameters keeps growing - I don't like that (I can do a PR to your PR if you like :-))

Regarding your "to discuss" points:

  • Imho the right prefix is picture ;-)
  • We could again, use my polyfill PR (Added universal polyfills for developers #7165). The picturefill polyfill could be added to https://github.com/kbjr/polyfill.js and would be available for anyone. You can disable either all the polyfills in the layout or by a simple unset() in the localconfig.php.
  • I think we have to evaluate a bit more on this one.
  • I'll review that when implementing the more object oriented Image class ;-)

Conflicts:
	system/modules/core/library/Contao/File.php
	system/modules/core/library/Contao/Image.php
	system/themes/default/main.css
	system/themes/flexible/main.css
@leofeyer
Copy link
Member

We should discuss the open issues on mumble. Do you happen to have time today?

@ausi
Copy link
Member Author

ausi commented Sep 26, 2014

Yes, is at 5 pm OK?

@leofeyer
Copy link
Member

What about 4:30 pm? Or anytime after 7 pm.

@ausi
Copy link
Member Author

ausi commented Sep 26, 2014

4:30 pm OK

@leofeyer
Copy link
Member

@ausi

  • Pass a DOM element to the picturefill() function
  • If src="" is the same as srcset="", output a default <img> tag and do not load the JavaScript

@leofeyer

  • Try to figure out a way to make loading the picturefill.js optional
  • Move the "extend the $GLOBALS['TL_CROP'] array" logic to the Controller class

@ausi
Copy link
Member Author

ausi commented Sep 27, 2014

@leofeyer done

@ausi
Copy link
Member Author

ausi commented Sep 29, 2014

@leofeyer Can you tell me shortly before you want to merge this PR, so I can merge the develop branch and resolve the conflicts?

@leofeyer
Copy link
Member

I will merge it tomorrow midmorning. You can merge now, because I will not be updating the develop branch anymore until then.

Conflicts:
	system/modules/core/library/Contao/Image.php
@ausi
Copy link
Member Author

ausi commented Sep 29, 2014

@leofeyer done

@ausi
Copy link
Member Author

ausi commented Oct 1, 2014

@leofeyer cropping SVG images should work now, see my last commit.

Should I create a new pull request for those changes on the feature/picture-element branch, or do you merge them by hand?

@ausi
Copy link
Member Author

ausi commented Oct 2, 2014

I reviewed the current feature/picture-element branch:

In Image::get() we should set the important part regardless of the value of $mode. E.g. Image::get('image.jpg', 100, 100, 'crop') should also use the important part to get a better cropping result. See madeyourday/contao-core@d719730

Image::getPixelValue('12pt') returns 9 instead of 16. The 12 / 16 in system/modules/core/library/Contao/Image.php:1323 shoud be replaced with 16 / 12.

Should we add a hint to the image sizes edit pages that people should enable the picturefill.js library? Or should we enable "Include picturefill.js" by default?

Everything else looks great 👍

@leofeyer
Copy link
Member

leofeyer commented Oct 2, 2014

Image::getPixelValue() fixed in ab462fb.

madeyourday/contao-core@d719730 merged in 3d6ee29.

I'll be adding the hint and then I'll merge :)

@leofeyer
Copy link
Member

leofeyer commented Oct 2, 2014

Hint added in 2350c88.

@leofeyer
Copy link
Member

leofeyer commented Oct 3, 2014

Finally merged in cd66d61. Thank you again @ausi and @Toflar for your great work!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants