Closed (fixed)
Project:
PhotoSwipe - Responsive JavaScript Modal Image Gallery
Version:
3.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
14 Jan 2022 at 15:12 UTC
Updated:
2 Mar 2022 at 08:09 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #3
balis_m commentedComment #4
balis_m commentedThis fix also solves the Cannot set image formatter to PhotoSwipe in Layout Builder.
Comment #5
balis_m commentedComment #6
marcoka commentedI have applied the patch and that fixes the referenced issue and now the formatter that i use in a display mode (/admin/structure/media/manage/image/display) works and the style is applied to the images i added inline using the ckeditor mediebutton with that display mode.
Thank you.
Comment #7
anybodyThank you all! My fear is that committing this fix breaks something else in other cases... So we please need test coverage for the future. Hope you understand, we're jumping from one problem into the next as the module is very widely used in different scenarios.
Comment #8
anybodyThis issue is blocked by missing test coverage for the photoswipe module.
Please first help to fix #2998694: Write tests for typical use-cases to reproduce this error, create a test and ensure the fix on this for all use-cases.
Hope you understand, we're jumping from one problem into the next as the module is very widely used in different scenarios.
See #2998694: Write tests for typical use-cases and help please. Until that, this issue is blocked.
Comment #9
anybodyAlso see the comment from @Berdir in
https://www.drupal.org/project/photoswipe/issues/3132556#comment-14255411:
Comment #10
anybodyThank you very much, I just reviewed and tested this and committed to 3,x-dev. Please try 3.x-dev now :)
Comment #11
dwisnousky commentedComment #13
balis_m commented@Berdir is right.
The check for the field_file_image_title_text is not needed anymore. I've created a new merge request.
Comment #14
anybodyThank you @balis_b, I'll have a look soon!
@dwisnousky what's that patch?
Comment #15
anybody@balis_m: Thank you, looks good! I replaced the !empty check by ternary operator, see: https://stackoverflow.com/a/49464633/10031165
Could you please add tests for checking the alt and title tag as of #2998694: Write tests for typical use-cases? We have to stop introducing regressions to this module due to missing tests.
Comment #16
anybodyStill needs review, as I made changes, I can not review myself. Please also test manually.
Comment #17
balis_m commentedAfter a manual test, I confirm that it works well.
Comment #18
anybodyThank you @balis_m, let's get this committed to dev. :) Great work!