[wip] Allow long captions #2
Conversation
andi34
force-pushed
the
master
branch
2 times, most recently
from
0c558bc
to
cc99a07
Compare
andi34
force-pushed
the
allow_long_captions-prettier
branch
from
67cdd34
to
2679c13
Compare
andi34
force-pushed
the
master
branch
5 times, most recently
from
cbe905f
to
a8277d0
Compare
…ement' to avoid confusion with the boolean option of the same name a few lines below. Also add some extra whitespace to separate functions.
…ption to go under the picture and be able to scroll to see it.
…lement and arrows and figure out why it is failing on mobile.
… so arrows are above expanded caption.
…top/bottom gaps so that caption function can be initiated with just the event. Not sure if this is better or just different.
…tion toggles now. Adding/removing whitespace for clarity.
…to indicate the end. Make caption text white so people like me can actually read it.
…apping on the caption because it is too easy to close accidentally when you thought you were swiping.
…nd/collapse arrows without another request to get an image and still have it work on IE11. Now need to get the event to fire when clicking on the arrow itself. Not sure it is worth the trouble.
…ize set explicitly so that it will work with IE11.
…n toggling the control.
…viewport is wide enough.
…o where the keydown events are handled. Also removing some console output.
Note that Google says that doing a CSS animation on height (and width) is bad for performance so I may revisit this later but it seems to work just fine. https://developers.google.com/web/updates/2017/03/performant-expand-and-collapse
…her than attempting to manipulate an existing one.
…n a phone does not call the goTo function so the resetCaption function was not getting called which had some odd results.
Unfortunately it did not work reliably and so it is now commented out in the hope that someone else will understand how to fix it. Added lines 348 and 719 as I thought they might be why I could usually expand the caption with an upward swipe but could not close it. It seems those lines should be there but they did not seem to help so now commented out.
…en when allowLongCaptions is true. Don't close caption on vertical drag when allowLongCaption is true. Don't close caption when tap is on an element inside the caption
andi34
force-pushed
the
allow_long_captions-prettier
branch
from
2679c13
to
17bf594
Compare
|
If caption expanded and next picture contains a caption too it can not collapse the caption. |
src/js/ui/photoswipe-ui-default.js
Outdated
| @@ -59,14 +59,14 @@ | |||
|
|
|||
| // If allowLongCaptions is true, position caption just under picture and show "Expand" button if necessary | |||
| if (_options.allowLongCaptions) { | |||
| var imagePositionTop = item.initialPosition.y; | |||
There was a problem hiding this comment.
It can't get item.initialPosition.y somehow
There was a problem hiding this comment.
Uncaught TypeError: Cannot read property 'y' of undefined
There was a problem hiding this comment.
This is a mystery since it was working for me earlier.
There was a problem hiding this comment.
I am only using the title attribute, could this cause an issue? (See the index.html)
Have a good flight!
There was a problem hiding this comment.
@andi34 I remembered to look at this again. In my rush to get out the door for our trip, I didn't understand the issue. My long caption solution has the captions - both the teaser and the full version - in
elements (one with class="figure__caption--thumbnail" and the other "figure__caption--fullsize") using the structure shown here https://github.com/dimsemenov/PhotoSwipe/blob/master/website/documentation/getting-started.md#-how-to-build-an-array-of-slides-from-a-list-of-links and then the Javascript harvests that information to create the options object. Aside from being easier (for me) to generate the HTML, instead of the JS that includes the options object, from the database , putting the information in the figcaption elements means that it can contain arbitrary HTML so links and even other images can be included.I'm going to update the documentation to make that clear and also check that I have not broken it for those using only the title object for the standard implementation. I wish I had done that earlier since now I can barely remember how it works!
I noticed in commit 83885ed that you moved some declarations further up. I had it where it was so that code would only be exercised if _options.allowLongCaptions was true.
Do you have a public-facing test page? When I ran your index.html I did not see any captions.
There was a problem hiding this comment.
Thanks for your reply!
There's a test page inside the source, only dist files haven't been generated.
You could clone my fork and checkout the "allow_long_captions-prettier" branch and generate them. I can do also and push to that branch if needed.
https://github.com/andi34/PhotoSwipe/blob/allow_long_captions-prettier/index.html
Without 83885ed it wasn't working and had some errors visible in the browser console. My fork is far ahead of the official tree, have added a lot of stuff from the open Pullrequests.
Thanks a lot for looking at it, let me know if you need anything :)
src/js/ui/photoswipe-ui-default.js
Outdated
| var gapTop = item.vGap.top; | ||
|
|
||
| ui.resetCaption(); | ||
| var naturalCaptionHeight = innerCaptionElement.clientHeight; | ||
|
|
||
| var captionCtrl = captionElement.querySelector('.pswp__button--caption--ctrl'); | ||
| var captionCtrl = document.querySelector('.pswp__button--caption--ctrl'); |
There was a problem hiding this comment.
Doesn't work without this...
|
I think using title attribute shoud be OK. It seems an appropriate use. About to board plane now.
…-------- Original Message --------
On Sep 5, 2020, 14:57, Andreas Blaesius wrote:
@andi34 commented on this pull request.
---------------------------------------------------------------
In [src/js/ui/photoswipe-ui-default.js](#2 (comment)):
> @@ -59,14 +59,14 @@
// If allowLongCaptions is true, position caption just under picture and show "Expand" button if necessary
if (_options.allowLongCaptions) {
- var imagePositionTop = item.initialPosition.y;
I am only using the title attribute, could this cause an issue? (See the index.html)
Have a good flight!
—
You are receiving this because you were mentioned.
Reply to this email directly, [view it on GitHub](#2 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/ABPUROFZZA5XDRM4RDARZGLSEI7THANCNFSM4P4OBIRA).
|
andi34
force-pushed
the
allow_long_captions-prettier
branch
from
07ff53d
to
2e70369
Compare
Long caption solution by @PeterSweetAndSour
dimsemenov#1692