[5.x] Add photoswipe options UI (and optionally setting presets)

I guess this should remain postponed until https://www.drupal.org/project/photoswipe/issues/3232070 ✨ [5.x] PhotoSwipe 5 Branch Fixed will be merged, because v5 of photoswipe provides new options, and we should wait until this issue will be merged.

Photoswipe 5.x released! We can tackle this issue once again!

@imclean, the thought about different option sets sounds very interesting! Especially for a formatter such as Photoswipe! 🙂

We could create a config entity class named "PhotoswipeOptionsSet" with its own collection, where new options sets could be created and removed. One default "PhotoswipeOptionsSet" is always created on install with the default options set. And people could select the options set they want on the formatter settings page! (With a link to the collection to create new options sets). This way, we don't "overload" the formatter settings page, as we already have 4 settings to set for the formatter. Adding another 12? even only through optionally enabling a checkbox would be way too much. Instead, a simple 5th setting, where the user can select their options set to use, would be amazing!

As an alternative, we could always just simply create a global settings page, where we can modify the values globally and individually per formatter through a hook. Or individually through an "override global options" checkbox on the formatter settings, where we can override the global settings for this specific formatter.

But I'd prefer the first option more, actually! Even if it is more work.

From my perspective, the optionset thing is (by far) to bloated for Photoswipe. Typically, you may want consistent UI within a project, which means the same functionality and look for the whole project.

I'm not against implementing this, but we as maintainers won't invest time into this functionality. I even think if someone does, it should be done as a submodule to keep the module simple for 95% of the use-cases.

For now, let's focus on the simple configuration page, there's enough more important things to be done! :)

(If important enough for someone, please split that larger part off into a separate feature request)

Yea, I don't agree with the UI part either. Like I said in my comment, I'd rather use config entities with the predefined "collection" page, similar to the COOKiES module:

But instead of services, we have "Options Sets". And these would be simply selectable through a "select" on the formatter settings page. This way we don't bloat the formatter settings page and users can use their different options set for different fields!

But I understand that this:

As an alternative, we could always just simply create a global settings page, where we can modify the values globally and individually per formatter through a hook [if necessary.]

Is the faster (and saner) approach.

On it,
implementing a global settings page with all the already given ones in the schema.yml.

Are all options from https://photoswipe.com/options/ already present in the schema.yml? (Excluding the JS callbacks - these should be made available differently, perhaps exposing the globally initialized Photoswipe object to Drupal.photoswipe in the initializiation?

@Grevil: I think that should be a separate issue / follow-up? Would you mind having a look at that idea and creating the issue, perhaps also related to the thoughts and last comment from ✨ [5.x] Make photoswipe library attachable in theme ({ attach_photoswipe() }) Fixed ?

Settings form created in general,
left out two settings that I was unsure about, since they are not documented on photoswipe.com.

@LRWebks:

left out two settings that I was unsure about, since they are not documented on photoswipe.com.

Which ones?

Left out the settings "maxZoomLevel" and "preload", which I have also left as a comment within the PhotoswipeAdminSettings.php

@LRWebks:
Re #23: I found the documentations!

• maxZoomLevel: https://photoswipe.com/adjusting-zoom-level/
• preload: https://photoswipe.com/options/#preload

Please add them as described in the docs.

I also opened an issue to add "maxZoomLevel" to the options page: https://github.com/dimsemenov/PhotoSwipe/issues/2052

Some values are missing in the schema, the install file and here in the settings form. The one's that make sense, should be added. See comment.

See here for details: https://github.com/dimsemenov/PhotoSwipe/blob/31c8a855170fc747db08445cb0...
https://photoswipe.com/options/

Missing configs (with their default values to use and the form element type to use):

• showAnimationDuration: 333 (integer input)
• zoomAnimationDuration: 333 (integer input)
• returnFocus: true (checkbox)
• maxWidthToAnimate: 4000 (integer input)
• clickToCloseNonZoomable: true (checkbox)
• imageClickAction: 'zoom-or-close' (select of all allowed values)
• bgClickAction: 'close' (select of all allowed values)
• tapAction: 'toggle-controls' (select of all allowed values)
• doubleTapAction: 'zoom' (select of all allowed values)
• indexIndicatorSep: ' / ' (text input)
• preloaderDelay: 2000 (integer input)
• easing: 'cubic-bezier(.4,0,.22,1)' (text input)

index should not be added.

We should also rename "PhotoswipeAdminSettings" to "PhotoswipeSettings", since the naming is very irregular inside the module (schema definition is "photoswipe.settings", settings form id is "photoswipe_admin_settings", etc.). Please resolve this naming issue as well.

Added all the settings that @Anybody proposed from the photoswipe website and took into account most of his other suggestions and comments.
Note that the settings names in the form (not the ones that the values are saved under) needed to have the "." replaced with underscores in order for the page to properly work.
Not finished yet, will write a test for the settings page before putting the issue into review.

Added and ran a test for the Settings page (It works, by the way). This concludes my work here, I will put this issue into review now!

@LRWebks: 4 failing tests, back to needs work:
https://www.drupal.org/pift-ci-job/2740648 →

Quickly fixed a few errors, among these a refactoring error (GeneralPhotoswipeTest.phph was named class PhotoswipeSettingsTest), and I hope it works now!

Hello, from a quick review, I see that hook_update_N is missing. As we introduced new options we, also need to update the configuration in existing installations.

Hm crazy, for some reason 'allowPanToNext' always stays FALSE inside the photoswipe object... No idea if this might be a library bug.

@Grevil: In the Photoswipe library it's even true by default.
See https://github.com/search?q=repo%3Adimsemenov%2FPhotoSwipe%20allowPanToN...

So we should check that more in deep. One typical thing is type conversion bool vs. string etc. from PHP vs. JS.
'true' vs. TRUE vs. true in PHP / JS - do we have a bug here?

@Anybody, as discussed internally, this was the problem:

    if (!this.supportsTouch) {
      // disable pan to next slide for non-touch devices
      pswp.options.allowPanToNext = false;
    }

I'll adjust the 'allowPanToNext' description accordingly.

Ok, we still need to flush the correct cache after submitting the settings form. Otherwise, photoswipe will simply use the old cached values as the Functional Javascript test perfectly shows.

@LRWebks please check the remaining comments and adjust them accordingly on Monday. Afterwards @dinazaur or me will make a final review.

Oh, seems I broke something. I'll have a look tommorow.

Ok, should be fixed again.

Ok, I finally adjusted a few of the descriptions and fixed / tested the update hook. Everything works as expected!

Nice work @LRWebks, let's merge this, once the tests pass.

Just needs rebasing and we can merge it.

Merged!

Please ignore the last couple commits in here. I reverted them and simply rebased.

I'm sorry but I reviewed the task a few times, is there any particular reason why you didn't credit me on the issue?

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

Sorry @dinazaur I missed that. Corrected it now!

Damn, didn't know reviewing should also grant credits, but it does → . I think I lost quite a bit of credits in the past!

My apologies.