-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Experiment: Add lightbox to Image block using directives #49621
Experiment: Add lightbox to Image block using directives #49621
Conversation
It will be essential to ensure that all the accessibility requirements are met. I added the
That should be a good starting point for the discussion. It's very likely that the link that triggers the lightbox doesn't need to be visible until someone uses the assistive technologies. In effect, it could use the The Navigation block can also be used as a reference because when it's displayed as a small menu icon, it opens an accessible overlay with the close button. |
@jasmussen Great, thanks for the feedback!
Ok got it. This will likely come along with some more work on behaviors that I or someone else may take on — we can keep this in mind as we devise those next steps.
Thanks for the feedback! I can make these changes and address these issues 👍
This was something mentioned during a discussion with @luisherranz and @SantosGuillamot as well. I believe the end goal is to incorporate this into Global Styles just like you mention here, though a first pass would be to just allow users to enable the lightbox at the theme level with theme.json.
I think this sounds great. A heads up that this lightbox behavior is based on the behavior used at Medium.com. I've set up a test article here, though you can also go to any Medium article and click on an image to see it. On our end, I believe I'll need to calculate the width of the screen to set the image scale. One question I have: How do you propose we handle this behavior on mobile? While the Medium behavior works well on desktop, it's a bit wonky in mobile web: Mobilemedium-mobile.mp4Desktopmedium-desktop.mp4 |
A lightbox is basically just a modal dialog wearing a costume, and it should follow the same accessibility protocol that any modal needs to follow. If this was built on top of existing modal implementations, that would probably be a good plan. |
Before you make too many changes to where the toggle lives, would be good to check with @SaxonF, who I think had a design putting this in the link dialog. Furthermore, on reflection I'm not sure it's valuable as a style option after all, since it's a bit more behavioral in nature — definitely a judgement call. The main motivation to moving it to a block supports was to surface it in Global Styles, so that it could become a single "set it and forget it" toggle: make all images open a lightbox unless linked. Saxon what do you think? Are there use cases where you would want to disable this? Also CC: @jameskoster as you've thought about global settings like these. Is there a way to make it trivial for Artemio to move forward, but still in the future make it a global toggle? Maybe it can continue to live in "Advanced" for now? |
I can't get this working for some reason. I see the "Lightbox" checkbox in the Advanced panel, but toggling it on does nothing on the frontend. Tried adding/removing links in the link control – no dice 🤔
This seems sensible to me. Otherwise it's not really clear which behaviour would take precedence if you were to enable lightbox and add a link (either to media file or attachment page). On that note, it may make sense to see this option only when a link to the media file is added? I don't have a strong feeling about the global implementation, but on the whole it feels like more of a contextual affordance, similar to making a link open in a new tab. I'd probably lean more towards a user preference for choosing the default behaviour when inserting images, rather than a global setting that gets unilaterally applied across the site. |
My main concern with adding it to the link dialog is that if I wanted to retroactivelly enable it on every image on my site with thousands of posts, wouldn't I have to edit them all? |
Yes, that's the trade-off. It feels like a separate thing to me though, and could be handled in a plugin. If we did make this a global style property, what would happen if an image is set to link to the attachment page, and lightbox is true? |
If an image has any kind of clickable link, my take is the lightbox should just not fire ever. So I could see a single toggle "show lightbox for images" somewhere, and then if an image is linked, it just won't fire there. |
That sounds a bit confusing to me, especially as a 'style' property. 🤔 |
Yeah, style may not be the best place for it. That said, lightbox for only unlinked images seems to be the industry standard as far as my experience goes. Have you seen otherwise? |
Some members of the design team spoke about this recently in a huddle. The global element gets quite tricky, and probably needs more design. Is it a site setting, if so where does that live? Is it part of global styles, and if so how do we make the interactions with link control feel intuitive? How do we enable local overrides at different levels (page / template / pattern / block)? With all the said, the best course of action may be to design a really good UI for the most atomic scenario, IE a single image block, and extrapolate out from there when we've had more time to consider the global scope. What do y'all think? |
Has it been considered, instead of a zoom in motion, which in my opinion can be a bit too much, something like this? image_effect.mp4image_effect_2.mp4And better yet, the possibility to define the animation and background color? 🙂 |
I like this approach better. It's much more straightforward to implement and, as I'm looking at the code, I think it would be challenging to get the zoom to animate consistently across all WordPress sites. |
I also prefer Bea's animation 😄
That's a great question, and I think we haven't thought about it. I guess that if we add a setting to customize I would suggest leaving that for a second phase. Once we have the first version polished, we can start thinking about adding support for customization and adding some more animations. |
One thing that might thread the needle is to use the same scrim color as the site background color, at least for the initial version. That takes care of dark or light themes at least. |
@jameskoster Do you have the |
I pushed up a change to the animation so that it fades instead of zooming: lightbox-fader.mp4Next will look at accessibility while discussion on the UI continues. |
I prefer the zooming, though if the consensus is to fade that's fine. 🤷♂️ |
While testing today I noticed the front-end admin bar disappearing if the block/control is present: CleanShot.2023-04-19.at.17.04.15.mp4 |
I just pushed a commit that adds accessibility. lightbox-accessibility.mp4
I tested with a screen reader, and I think it provides ezough detail but I'm not sure — would appreciate a second pair of eyes on that 🙏 Next will work on the admin bar bug and the dynamic scrim color. |
b20351a
to
58b0ef5
Compare
const raf = window.requestAnimationFrame; | ||
// Until useSignalEffects is fixed: https://github.com/preactjs/signals/issues/228 | ||
const tick = () => new Promise( ( r ) => raf( () => raf( r ) ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this pull request has been merged, this workaround shouldn't be necessary. Could you please try merging the add-interactivity-runtime
branch and check if everything works as expected without using await tick()
?
@@ -111,6 +111,7 @@ export function ImageEdit( { | |||
width, | |||
height, | |||
sizeSlug, | |||
enableLightbox = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure but, shouldn't we define the default value in the block.json
instead of here? Something like:
"enableLightbox": {
"type": "boolean",
"default": true
}
06cdbd9
to
0cdab08
Compare
Looks like this was closed when the base branch was merged; reopening now. |
@artemiomorales, maybe try to rebase with |
Hi all, it appears we'll be unable to recuperate this pull request, so I've opened a new one here. Let's continue the conversation there. Thanks! |
Hi all, here I'm giving a call for feedback as I've just pushed a batch of changes to the new pull request. The admin UI will change slightly when it's synced with WIP: UI Behavior, but other than that, I believe this PR is done. Would be great to hear feedback on any of the decisions or revisions made 😄 |
What?
This is part of the experiments section with the interactivity API in revising core blocks.
It uses directives from the Directives hydration system
Why?
It would enable native support for image lightboxes, a commonly used feature in many websites, without the use of jQuery or other external libraries.
How?
It adds directives on click to images, presenting a larger version of the image along with an overlay, which can be hidden with an additional click, by scrolling, or pressing the escape key.
Testing Instructions
Testing Instructions for Keyboard
Notes
Screenshots or screencast
Editor
image-lightbox-edit-mode.mp4
Frontend
image-lightbox-1080.mp4