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

Call new pre_scale and get_or_generate from plone.scale. #113

Merged
merged 46 commits into from
Jun 9, 2022

Conversation

mauritsvanrees
Copy link
Member

See branch pre-scale of plone.scale, in plone/plone.scale#57.

See branch pre-scale of plone.scale.
Only do this when pre=True, so when generating a tag.
You can override the standard behavior by explicitly passing new boolean parameter include_srcset.
@MrTango MrTango self-requested a review May 17, 2022 10:46
Show some versions of the image with different directions.
Show stored scales.
Add button for clearing the scales.

Currently only works when the fieldname is 'image' and there is an actual image.
Avoids a weird undefined genericsetup:importStep in CMFCore/exportimport/configure.zcml
Otherwise the scale method thinks we only want the current actual image scale, and then the srcset info would be useless so it does not include it.
This shows we do not get the generated scale for the srcsets anymore.
@mauritsvanrees mauritsvanrees marked this pull request as ready for review May 18, 2022 15:34
@mauritsvanrees
Copy link
Member Author

Ready for review, but I need to fix tests in plone.formwidget.namedfile and plone.restapi.

@MrTango
Copy link
Contributor

MrTango commented May 20, 2022

@mauritsvanrees i added a fix here and in plone.scale to prevent some errors the modified parameter was producing in some methods

For example when adapting a Folder:

```
TypeError: http://localhost:8080/Plone/@@ajax-search
Traceback (innermost last):
  Module ZPublisher.WSGIPublisher, line 167, in transaction_pubevents
  Module ZPublisher.WSGIPublisher, line 376, in publish_module
  Module ZPublisher.WSGIPublisher, line 271, in publish
  Module ZPublisher.mapply, line 85, in mapply
  Module ZPublisher.WSGIPublisher, line 68, in call_object
  Module Products.CMFPlone.browser.search, line 288, in __call__
  Module Products.CMFPlone.browser.search, line 303, in get_image_tag
  Module plone.memoize.volatile, line 74, in replacement
  Module plone.namedfile.scaling, line 600, in tag
  Module plone.namedfile.scaling, line 582, in tag
  Module plone.namedfile.scaling, line 510, in scale
  Module plone.scale.storage, line 220, in pre_scale
  Module plone.namedfile.scaling, line 200, in get_original_value
  Module zope.component.hooks, line 135, in adapter_hook
  Module plone.dexterity.primary, line 24, in __init__
TypeError: ('Could not adapt', <Folder at /Plone/news>, <InterfaceClass plone.rfc822.interfaces.IPrimaryFieldInfo>)
```
This does actually return None in some cases.
@MrTango
Copy link
Contributor

MrTango commented Jun 1, 2022

@mauritsvanrees i started with the picture tag method and the refactoring of the needed code in plone.outputfilters.
I created a branch from your branch called pre-scale-picture-tag and added it to the plip file.
I also added a demo to your test.pt file which shows already a working version.
I'll continue to work on this to coming days.
Some functionalities are missing, like the extra parameters the tag method supports.
We need tests to be fixed, since the markup of the picture tag has changed in over time and is not updated in there.
For plone.namedfile i started writing tests, but got errors because plone.registry could not be used in tests.
Maybe we can look at this together soon.

@MrTango
Copy link
Contributor

MrTango commented Jun 2, 2022

Added some fixes and examples, it should work now here and in outputfilters

@MrTango
Copy link
Contributor

MrTango commented Jun 2, 2022

I added title and alt attributes to the picture method, as well as examples for it and added a changelog entry

MrTango and others added 19 commits June 3, 2022 19:42
This does a very small start for fixing the tests.
We do not have plone.base there.
The picture method is not supported on 5.2, but it would be nice if the rest of the code still works.
I think get_picture_variants is mocked away in all tests, and this helps us.
…und.

When a template calls the picture method, it seems nicer to return a normal img tag in case of problems.
This also helps for Plone 5.2.
@mauritsvanrees
Copy link
Member Author

I have merged the pre-scale-picture-tag branch in here.

@mauritsvanrees
Copy link
Member Author

All green in combination with plone/plone.scale#57.
Since no one has objected, I say we merge this tomorrow (Thursday). Then the work on picture variants and on image scale info in the catalog can continue with less branches of packages.

@ale-rt
Copy link
Member

ale-rt commented Jun 9, 2022

I still had no chance to look at this but I fully trust you both :)

@mauritsvanrees mauritsvanrees merged commit 30f59c5 into master Jun 9, 2022
@mauritsvanrees mauritsvanrees deleted the pre-scale branch June 9, 2022 11:18
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.

3 participants