Lightbox: Addition of PhotoSwipe 5 (Vanilla JS) #209
Conversation
Bumped version to 3.0.0. Integrated PhotoSwipe 5.x vanilla JS library. Removed jQuery requirement for PhotoSwipe mode. Added aspect-ratio logic to prevent layout shift. Implemented root-appended responsive captions.
|
Hi Onli! |
|
Hi @jmglastetter, Thank you for the PR! I added a commit to your branch, changing the line ending from CRLF (windows default) to LF (linux default and the format of the original file). That way, the diff comparison works again and I can more easily go though the changes. There might be a setting in your editor to stick with the original line endings control character, to avoid this :) Alternatively there is a git setting, https://stackoverflow.com/questions/10418975/how-to-change-line-ending-settings, where you could change it on commit to LF and work locally as you prefer. The I'll go through the changes in the PR and leave questions if I stumble upon something. If there are changes to do, I'll ask for them - if they can't be done or it's too much please say so, then I will merge and help with the changes if they are really necessary (I wrote this before seeing how much, or in this case how little, needs to be done :) ). I already checked the demo, the photoswipe lightbox feels great both on desktop and on mobile, plus the plugin needs a maintainer, so we'll definitely merge this :) |
There was a problem hiding this comment.
This looks good! I added some code questions, but I think only the block -> inline-block change for lightbox is really needed to be cleared up. And I'd restore the removed comments to make future maintenance easier.
The other remaining task is editing the ChangeLog file of the plugin, to document your changes. Could you add this to the PR?
Otherwise please just have a look at my comments, and then let me know if this is ready to be merged.
| $propbag->add('description', ''); | ||
| $propbag->add('select_values', array('colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); | ||
| $propbag->add('default', 'lightbox2jq'); | ||
| $propbag->add('select_values', array('photoswipe' => 'PhotoSwipe (No jQuery)', 'colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); |
There was a problem hiding this comment.
The "(No jQuery)" is not necessary here, let's call it just "PhotoSwipe"
There was a problem hiding this comment.
Ah, I just noticed: This is a possible migration bug. I think selected values are remembered by their index (I'm certain that is true for a multi select widget in the plugin configuration, but am not certain it was this one). So adding photoswipe as a first option would move what users have configured.
Easiest to add it as the last option instead, then existing configurations will work as before. Also fits here alphabetically.
| }// do not use 'class' here as identifier, whenever possible, since this conflicts/not validates with $1 'class'es | ||
| else { // force new lib to prevent empty regular expression errors in preg_replace() | ||
| $type = 'lightbox2jq'; | ||
| $this->set_config('type', $type); |
There was a problem hiding this comment.
This seems like a strange workaround. You removed it by choice, judging it to be unnecessary?
| case 'frontend_header': | ||
| $headcss = true; | ||
| case 'frontend_footer': | ||
| // case plugin imagesidebar on eg staticpages. We need the libs then! |
There was a problem hiding this comment.
Hm, this comment seems helpful, it should stay
| $check_imagesidebar = serendipity_plugin_api::enum_plugins('*', false, 'serendipity_plugin_imagesidebar'); | ||
| $cisb = (is_array($check_imagesidebar) && $check_imagesidebar[0]['placement'] != 'hide') ? $check_imagesidebar : null; | ||
|
|
||
| // If no imagelink was processed, don't add css or js files to the header or footer! (configurable plugin option) |
There was a problem hiding this comment.
Same here. I assume the editor autoremoved this? You can of course remove old comments if they are a hindrance, but I assume this was a mistake, and some of those explanation would really be helpful. There are some more removed comments below, can they stay? :)
| .serendipity_image_link { | ||
| display: block; | ||
| .serendipity_image_link, .pswp-enabled { | ||
| display: inline-block; |
There was a problem hiding this comment.
This also works for the old lightbox option?
| } | ||
|
|
||
| function install() { serendipity_plugin_api::hook_event('backend_cache_entries', $this->title); } | ||
| function uninstall(&$propbag) { serendipity_plugin_api::hook_event('backend_cache_purge', $this->title); } |
There was a problem hiding this comment.
This removes serendipity_plugin_api::hook_event('backend_cache_entries', $this->title);. I do not know if it's necessary, but the change was intended?
There was a problem hiding this comment.
The prior example() function was misnamed, it was a cleanup function. Completely okay to remove it, the old files hurt no-one. But better to remove it completely, no?
| function install() { serendipity_plugin_api::hook_event('backend_cache_entries', $this->title); } | ||
| function uninstall(&$propbag) { serendipity_plugin_api::hook_event('backend_cache_purge', $this->title); } | ||
| function example() {} | ||
| private function empty_dir($dir) {} |
There was a problem hiding this comment.
Here as well, I'd recommend to remove empty_dir completely. It's not needed for a valid serendipity plugin.
| $propbag->add('description', ''); | ||
| $propbag->add('select_values', array('colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); | ||
| $propbag->add('default', 'lightbox2jq'); | ||
| $propbag->add('select_values', array('photoswipe' => 'PhotoSwipe (No jQuery)', 'colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); |
There was a problem hiding this comment.
Ah, I just noticed: This is a possible migration bug. I think selected values are remembered by their index (I'm certain that is true for a multi select widget in the plugin configuration, but am not certain it was this one). So adding photoswipe as a first option would move what users have configured.
Easiest to add it as the last option instead, then existing configurations will work as before. Also fits here alphabetically.
| $propbag->add('select_values', array('colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); | ||
| $propbag->add('default', 'lightbox2jq'); | ||
| $propbag->add('select_values', array('photoswipe' => 'PhotoSwipe (No jQuery)', 'colorbox' => 'ColorBox', 'lightbox2jq' => 'Lightbox 2 jQuery', 'magnific' => 'Magnific-Popup')); | ||
| $propbag->add('default', 'photoswipe'); |
There was a problem hiding this comment.
This change however should cause no issue.
2 participants
This PR migrates the PhotoSwipe version 5.x, enabling a zero-dependency (Vanilla JS) lightbox experience. It includes fixes for Cumulative Layout Shift (CLS) by pre-calculating thumbnail aspect ratios and introduces a modernized, responsive caption system.