[5.x] PhotoSwipe 5 Branch

Hello.

So, it's ready for review. What was done:

1. I've removed/adjusted obsolete configuration options. And added new ones for the Caption plugin. Check photoswipe_update_9001.
2. Implemented the ability to use the Caption plugin.
3. Fixed all tests.
4. Replaced esm photoswipe library with umd as suggested in #14

The only problem that remains is Caption plugin, I didn't manage to find CDN with umd (it exists in the repository), So I used UMD for this plugin. And if someone will want to replace it will local library, it is impossible for now. For this, we will need to add another dependency that users will need to download with the composer.

P.S. I forgot how to run tests in MR, can someone execute them?

MR has conflicts, can't be merged

I'm not seeing any merge conflicts with the MR currently. Setting back to "Needs review"

@dinazaur looks great!

Tested manually upgrading from 3.x and no issues found.
Note: I found cdn link with umd version for caption here https://unpkg.com/photoswipe-dynamic-caption-plugin@1.2.7/dist/photoswipe-dynamic-caption-plugin.umd.min.js. Looks umd version was added into 1.2.7 release.

MR has conflicts, can't be merged

@elaman, I can't also apply patch by using patch link (....14.patch), so I've changed url to use .diff instead and now apllied as expected

@vlad.dancer thank you. The MR works, so +1 on RTBC

Grevil → closed merge request !75

Grevil → closed merge request !76

Thanks, everyone, for your immense work on this issue!! 🤩

I made a few comments in code, this looks a lot, but it is mainly about moving the dynamic captions part into its own submodule 🙂. I also tried getting around rebasing everything again. Unfortunately, this wasn't possible.

Unfortunately, we currently don't have much time to implement anything by ourselves. But I hope my comments will lead in the right direction!

Hello, since that's not my PR I cannot mark discussions as resolved. Could someone clean up discussions there?

@dinazaur
Wow

        // Adds ability to react on photoswipe initialization.
        const event = new CustomEvent('photoswipeLightboxBuild', {
          detail: {
            lightbox,
          }
        });

Wow! You are reading my mind! Awesome!
Will check new PR today.
--
Oh, I got you, this is for photoswipe_caption.js but could be useful for extending galleries in custom modules (for example adding configuration functions for zoom level, etc).

          initialZoomLevel: (zoomLevelObject) => {
            const {w, h} = zoomLevelObject.itemData;
            let zoom = 'fit';
            if (w > h) {
              zoom = window.innerWidth / w;
            } else if (w <= h) {
              zoom = window.innerHeight / h;
            }
            return zoom;
          },

Right now I'm just altering photoswipe library definition and replacing with custom implementation.

But could do using events having the same event for caption plugin to put counter inside caption region:
new CustomEvent('photoswipeDynamicCaptionBuild', {detail: caption});

         captionContent: (slide) => {
            const prefix = `${slide.index + 1} / ${slide.pswp.options.dataSource.items.length}`;
            let title = slide.data.element.getAttribute('data-overlay-title');
            return `<span class="counter-wrapper">${prefix}</span>` + title;
          },

What do you think?

Hi @vlad.dancer,
I'm not sure how to alter option with your solution: new CustomEvent('photoswipeDynamicCaptionBuild', {detail: caption});

There is no documentation of how to do it, and there is no "source" code to check what's going on under the hood. Only minified versions.

Another way would be to pass options instead of the caption instance itself. E.g. new CustomEvent('photoswipeDynamicCaptionOptions', {detail: captionOptions});

That would be good for users who want to alter individual photoswipe instances. In that case, we will also need to add same event for photoswipe options too.

But even now you can alter them. You just need to make sure that your script is initialized before our and alter drupal settings. You can pass any options and they will override ours.

{
      captionContent: (slide) => {
        return slide.data.element.getAttribute('data-overlay-title');
     },
     ...captionOptions,
}

As you can see we merge options from settings, so captionContent will be overridden if you will pass your custom callback there.

Immense work @dinazaur! 😯

I overflew the changes, commented and resolved remaining threads!

@vlad.dancer feel free, to create a separate follow-up issue for your feature request in #36!

There are still a few points we need to resolve:

• The cdn implementation is almost perfect, but in its current state still a bit bothersome. Instead of automatically using the cdn library, if the library isn't stored locally, we should instead let the user manually enable a checkbox on the settings page to "enable cdn". This setting should be disabled on default. This way we make sure a user doesn't accidentally fetch the library via cdn, because he forgot to require the library locally.
• We need to make sure the module is perfectly upgradable from 3.x to 5.x without any problems
• We need to make sure we do not implement any regressions
• We have to make sure the test coverage of the module is good enough for this huge update. @dinazaur How would you rate the current test coverage? Is it enough to make sure everything still works as expected?

But once again, impressive work! Let's keep this going! :)

Thanks, just one addition:

Instead of automatically using the cdn library, if the library isn't stored locally, we should instead let the user manually enable a checkbox on the settings page to "enable cdn".

A checkbox wouldn't be good UX for this, better use radio or select. But yes, please explicitly, because of the GDPR risks, if fallback is used.

dinazaur → closed merge request !77

I don't have permission to run tests on current MR, tried to create separate branch and open new PR, but even there I don't have permission. So attaching patch here, to check if tests are running correctly.

The last submitted patch, 44: 3232070-support-photoswipe-v5--44.patch, failed testing. View results →

My apologies! Automated testing was disabled for the newly created 5.x branch!

It's activated now, might take a bit though.

Nice, finally I managed to fix these tests 😅. What I implement is the ability to install photoswipe library locally inside Drupal CI environment. So we will make sure that composer installation is working fine. I changed all tests to use local library. And implemented one test that checks if CDN works fine.

The cdn implementation is almost perfect, but in its current state still a bit bothersome. Instead of automatically using the cdn library, if the library isn't stored locally, we should instead let the user manually enable a checkbox on the settings page to "enable cdn". This setting should be disabled on default. This way we make sure a user doesn't accidentally fetch the library via cdn, because he forgot to require the library locally.

Done

We need to make sure the module is perfectly upgradable from 3.x to 5.x without any problems

For this I think we can wait until more people will mark it as RTBC

We need to make sure we do not implement any regressions

Same as the previous point.

We have to make sure the test coverage of the module is good enough for this huge update. @dinazaur How would you rate the current test coverage? Is it enough to make sure everything still works as expected?

I guess it is fine enough. Also with new reworking tests, we are sure that locally installed library is working correctly.

Wow, that is wild @dinazaur!! Thanks a lot!

Changes are looking good! Still need to test it manually, in my local environment, when there is time, but just going over the changes everything looks very impressive! Great work! 🙂

LGTM!

(Letting this on "Needs Review" for other people to test this).

This please needs to be incorporated to not reintroduce that issue in 5.x: 🐛 Photoswipe library definition license is missing URL Fixed

I've added the Licence, as @Anybody asked. I've tested it on our website, marking it as RTBC. Thank you!

New Status Report Warnings:

and

Thanks @Grevil, nice work! I'd suggest to remove the 2nd. CDN simply shouldn't be enabled by default, then it's all fine with the first error message!

Please simplify the first line of the error to: "Photoswipe library not found, CDN disabled" that should be enough.

Aight!

Alright, the code and texts are cleaned up now! The final task will be, to move the Photoswipe field formatter caption settings into the newly added submodule!

This includes:

• photoswipe_caption
• photoswipe_caption_custom

from "PhotoswipeFieldFormatter", which should be (probably) added through formatter third party settings, using:
https://api.drupal.org/api/drupal/core%21modules%21field_ui%21field_ui.a...
and
https://api.drupal.org/api/drupal/core!modules!field_ui!field_ui.api.php....

And "getCaption()" from the "PhotoswipePreprocessProcessor", which should get encapsulated into a submodule service.

I'll finish this tommorow!

By the way, if we don't want to break compatibility I suggest not disabling CDN by default or leaving a clear warning on the new release that users who had not installed the library locally will have photoswipe broken.

@dinazaur, good call with the warning!

We definitely need to create a clear warning in the update hook, stating that the user needs to upgrade their photoswipe version to version 5, if they have installed the library locally OR enable CDN. But I'd vote against enabling it on update due to the before mentioned GDPR reasons.

But I'd vote against enabling it on update due to the before mentioned GDPR reasons.

+1 on this point, the legal thing seems to be the more important thing.

Composer does not update the library automatically?

@thomas.frobieter: No that's not possible, as it's an asset-packagist library and Drupal core doesn't know that repo. So it's always the administrator job to add it, if needed. Sadly, but there are reasons...

Ok, last commit was quite a big one! Requesting partial review by @Anybody, afterward back to "Needs work" for final cleanup and testing.

Please review: https://git.drupalcode.org/project/photoswipe/-/merge_requests/14/diffs?...

Back to NW, left some comments, but don't have the time for a full review in the next months. Please test with the communities help.

@Grevil do you want to fix the comments or will @dinazaur help perhaps?

Alright, all done! Further partial review please and afterward, I'll do some manual testing and we should be able to make an alpha1 release!

PS: I am still not sure about the drupalci.yml, as it could lead to unexpected test failure in the future, and already leads to different test output locally! Hence, I'd say we just plainly enable CDN for all tests (as before, using the "cdn as fallback" method) and be done with it.

I am still not sure about the drupalci.yml, as it could lead to unexpected test failure in the future, and already leads to different test output locally! Hence, I'd say we just plainly enable CDN for all tests (as before, using the "cdn as fallback" method) and be done with it.

Agree! drupalci.yml should be done in a separate issue anyway.

Let's tag an alpha1 and do a LOT of testing!
Please also add version information to the module page. 5.x should NOT be used in production, but please be tested a lot! alpha state!

Thank you all! Especially @dinazaur and @Grevil for all the work done here! Fabulous!!

Alright! I tested the module and its formatter settings extensively, as well as the upgrade path. All seems to work flawlessly! :)

Big thanks to @dinazaur, for the amazing and vast groundwork, to make this module compatible with Photoswipe 5! Really appreciating your work done here!

I think this is ready for an alpha1 release!

All Green! Let's do it! 🤹‍♂️

Automatically closed - issue fixed for 2 weeks with no activity.