Skip to content
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

Image block: Revise Lightbox UI to remove reference to Behaviors #53403

Closed
artemiomorales opened this issue Aug 7, 2023 · 48 comments · Fixed by #53851
Closed

Image block: Revise Lightbox UI to remove reference to Behaviors #53403

artemiomorales opened this issue Aug 7, 2023 · 48 comments · Fixed by #53851
Labels
[Block] Image Affects the Image Block [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Enhancement A suggestion for improvement.

Comments

@artemiomorales
Copy link
Contributor

What problem does this address?

As we've explored implementing an image lightbox, it's become increasingly clear that the feature is specific to the image, so we should revise the UI to be tailored to the image block accordingly instead of being encompassed under the concept of Behaviors.

What is your proposed solution?

Let's design a simplified UX for the lightbox that makes sense in the context of just the image block.

Questions:

  1. How are we going to surface the lightbox as a setting of the image block?
  2. How are users going to activate the lightbox?
  3. Will the lightbox come activated by default?
  4. Will it be a global setting for all images?
  5. How will the user deactivate the lightbox on a specific Image?
@artemiomorales artemiomorales added Needs Design Needs design efforts. [Block] Image Affects the Image Block [Feature] Interactivity API API to add frontend interactivity to blocks. labels Aug 7, 2023
@jasmussen
Copy link
Contributor

Indeed, experiences suggest the feature is mainly useful for images. This might require some deep thought, not just on lightbox, but on behaviors in general. CC: @WordPress/gutenberg-design

@jasmussen jasmussen removed their assignment Aug 8, 2023
@artemiomorales artemiomorales removed their assignment Aug 8, 2023
@SaxonF
Copy link
Contributor

SaxonF commented Aug 16, 2023

Does it make sense to just add a Lightbox option under image link?

image

@jasmussen
Copy link
Contributor

I think we discussed that on a previous issue. The benefit is that the lightbox should be mutually exclusive with hyperlinks, i.e. you should have one not the other.

The main challenge is that for images, much of the time the lightbox can/should be a "set it and forget it" thing that ideally you don't have to set on each image block. @jameskoster had some thoughts here recently.

@jameskoster
Copy link
Contributor

It would be good to determine how granular control needs to be.

  • A global setting caters to the "set and forget" idea.
    • A global setting per image – possibly controlled in the media library – is a helpful affordance for applying a behavior to all instances of a given image.
      • A local per pattern/container setting?
        • A local per block setting that overrides all of the previous options.

The last two seem the least useful to me, but I'm sure there will be scenarios where the per block control will be needed.

@Poliuk
Copy link

Poliuk commented Aug 16, 2023

  1. Will the lightbox come activated by default?

As many users might already be using other plugins to add something similar to our Lightbox to their images, what if we wait for 1 release to activate behaviors by default? So my proposal is that for 6.4, we would ship the lightbox disabled by default, and for the following WP release, we will switch it to activated by default.

@jasmussen
Copy link
Contributor

It’s on the radar to explore a great UI for, both long term and near term. That said, for the first version I would think we can lean into having only the Global Styles UI as a place where you can set itfor all image blocks in a central location. This is the is the most important thing to get right (and why it could make sense as a future exploration to explore this toggle living in a Settings page). There’s another exciting future challenge related to the UI (which overlaps with filters): what if I want both fade and zoom? This isn't blocking, but good to keep in mind for parallel UI design work.

It's also important is to have the Lightbox feature out there, as a way to test and refine the general behaviors system, so I’m keen to see it ship.

But one thing that would be really good to figure out for 6.3, however, is to only fire the lightbox when it’s relevant. Consider a testimonials page that features a slew of decorative logos in a row, just unlinked image blocks as you might see on the WordPress Enterprise page. In that case, even with the lightbox applied to images by default, it would be nice if those weren't zoomable. What’s a good heuristic to omit those from the lightbox? Can we fire the lightbox only if the image would actually be scaled up when zoomed? Can we fire it only if scaled up beyond a certain threshold?

@artemiomorales
Copy link
Contributor Author

Update

After reviewing the current implementation for the lightbox UX with the design folks across the theme.json, global styles, and image block settings, it looks like we'll be taking the following approach:

  • Remove the lightbox's theme.json support for now
  • Rename the lightbox labels to remove references to "behaviors" in the global styles
  • Revise the UI in the image block settings (we need a new design for this)

What’s a good heuristic to omit those from the lightbox? Can we fire the lightbox only if the image would actually be scaled up when zoomed? Can we fire it only if scaled up beyond a certain threshold?

Regarding this point, it's possible that we'd create confusion if the lightbox were enabled for some images but automatically disabled for others, so it seems like allowing users to configure this manually at the block level will be the approach we take.

@jameskoster mentioned being able to create a design for the revised UI in the image block settings and will aim to have an update on that next week 👍

@jameskoster
Copy link
Contributor

Here's a potential UI control for this:

lightbox
  • How familiar is “Lightbox” as a term? The label may instead read “Enlarge image on click” which would produce a shorter help text.
  • I added a “None” option for animation to facilitate simpler designs.
  • @jasmussen I know you mentioned combining animation effects, but I'm not sure we need that here? Zooming and fading together would look a bit strange imo.

In terms of placement, I lean slightly towards the Settings panel (rather than Advanced), but that’s not a strong opinion. Is it definitely too early to introduce a Behaviors panel?

Frame 11

@jasmussen
Copy link
Contributor

jasmussen commented Aug 21, 2023

Thanks for exploring. I could see something like this living in global styles, but for each individual image block, especially since in most cases this is a global "set it and forget it" setting, this feels much too prominent, both in terms of having its own panel, being visible by default, and even surfacing the animation.

Regarding that, behaviors have a potential to bring some magic to the frontend of block themes, where so far most of the magic has been happening on the back end. In some cases that means generic interfaces that can work across a variety of JS powered behaviors, like parallax or modal or something else, in other cases (like the lightbox) it really makes sense to have it only for the Image block. To that end, there are a few considerations here we need to balance:

  • It's too early to call this behaviors, since there's only one and it's localized to one block. I'd avoid the panel.
  • Just like you shouldn't set justified text on a single paragraph (if you want justified text, you probably want it default on all text blocks), so this seems like one of those settings you would not want to have to set for ever image you insert in your posts and pages. It seems like the best experience would be to set it once, globally, and then potentially have an interface for unsetting it locally.

Is it time for making the Settings panel actually into a ToolsPanel, and have the lightbox control be a non-default control which, according to your most recent idea around inheritance, could maybe be surfaced by default if the global setting is on by default?

Edit: looks like my comment had been truncated! I've edited for clarity.

@jameskoster
Copy link
Contributor

It's too early to call this behaviors, since there's only one and it's localized to one block. I'd avoid the panel.

That's fair.

Is it time for making the Settings panel actually into a ToolsPanel, and have the lightbox control be a non-default control which, according to your most recent idea around inheritance, could maybe be surfaced by default if the global setting is on by default?

Yes that's pretty much what I am proposing. So initially:

  • The control would be hidden in the local context regardless of the global setting...
  • But can still be added via Toolspanel to override the global setting – this is important.

I suppose this means it has to go in Settings (which is currently a Toolspanel for Image blocks). Advanced is not, and may never be.

When we have better inheritance handling in the future:

  • If lightbox is enabled globally, the control appears in the local context so that it's easy to disable.
  • Otherwise it's hidden, but can be added manually to enable lightbox ad hoc.

How do y'all feel about the toggle label? Is "Lightbox" familiar enough, or should we use the more explicit "Enlarge image on click" (or similar)?

@jasmussen
Copy link
Contributor

Sounds good, I guess the main open design question here, and this is the same issue for "Drop Cap", is: can we make it a one-click action to add the control from the ellipsis menu, rather than "add then toggle?"

I don't know that lightbox is clear enough, I'd agree on expanding that. Perhaps "Zoom in on click"? Keep it short enough to not wrap.

@jameskoster
Copy link
Contributor

can we make it a one-click action to add the control from the ellipsis menu, rather than "add then toggle?"

I think so. If the global setting is false and a user adds the lightbox control to an individual image, it seems safe to assume they intend to enable it.

Zoom in on click

Works for me. We can probably refine and potentially get the copy team involved in the PR.

zoom

@jasmussen
Copy link
Contributor

Close! Do we actually need the help text? I'd rather start without it and add it when we know it's needed. And should width/height/resolution show up in the tools panel dropdown? Even if they are defaults that can't be untoggled? Finally, while closed, this issue puts Alternative text in a collapsed separate panel: #38068

I'd also omit the animation picker here. I think it's too close to canonize it as a segmented control, as I could see this aspect evolving quite a bit to be almost global stylesy in nature. Just like you have a single place to change global typography, colors, spacing variables, you might also have a single place to define what types of animation are applied across both lightboxes and other aspects, and you'd need to be able to combine, mix, match, and perhaps even choose type of easing. Because this is such a rabbit hole, it seems best to either keep the drop-down in global styles only for now, or even just default to zoom until such a time as we are further with the system.

@jameskoster
Copy link
Contributor

Do we actually need the help text?

I think it's helpful, personally. Without prior technical knowledge it may not be clear whether link or lightbox behavior takes precedence. Not a strong opinion though. Maybe it's something we display as a notice (similar to color contrast warnings) when the user enables lightbox on an image with a link.

And should width/height/resolution show up in the tools panel dropdown? Even if they are defaults that can't be untoggled?

They currently appear, disabled, which I find to be a bit strange and unnecessarily noisy. We can potentially revisit that when we figure out inheritance. But that's a separate issue :d

I suppose animation will be a bit similar to box shadow, where we want to offer presets and the ability to customise fully. Happy to default to zoom initially while we work out those patterns, even if it prohibits the ability to disable animation in the short term.

zoom

@jasmussen
Copy link
Contributor

Good thoughts, both, and I can get on board with all of it.

@Poliuk
Copy link

Poliuk commented Aug 22, 2023

I don't know that lightbox is clear enough, I'd agree on expanding that. Perhaps "Zoom in on click"? Keep it short enough to not wrap.

We already debated renaming the Lightbox into something else. The conclusion was that we should keep the term Lightbox, so I'd prefer if we stick to that decision.

On top of that, I don't think "Zoom in on click" is accurate, we are not really zooming but "expanding the image on click". Sharing this just in case we want to add some extra explanation in addition to the "Lightbox" label.

@artemiomorales
Copy link
Contributor Author

@jameskoster Thanks for these designs!

To be clear, this is the design we'd see by default in the global styles, right?

262325069-53c5717e-3592-4144-979e-319a19f6d86a

And the toggle would be hidden by default in the block settings, but one could enable it like this? And this is the action one would need to take to disable the lightbox for a particular block?
image-setting

@hanneslsm
Copy link

hanneslsm commented Aug 22, 2023

Love the explorations here!

My 2c:

Thinking a step ahead, I could imagine a situation where users want extra options, such as toggling displaying the captions in the lightbox or changing the background color. With that in mind introducing a Behaviors (or "Lightbox") panel could already now make sense.

@michalczaplinski
Copy link
Contributor

I'd also omit the animation picker here. I think it's too close to canonize it as a segmented control, as I could see this aspect evolving quite a bit to be almost global stylesy in nature

Do we want to remove the animation picker altogether?

In case this wasn't clear before: The Lightbox's zoom and fade functionalities have already been fully implemented, so I'd prefer we find an acceptable UI to expose this choice to the users.

@jasmussen
Copy link
Contributor

I'm a bit skeptical of adding a dedicated panel for this, at least in the local context. We've worked long and hard to reduce the amount of bespoke panels in favor of reusable panels. If we do add a bespoke panel just for the image block, let's at least decide that it will form the foundation of a future generic "Behavior" panel, like Jay suggested initially.

To that end, I personally still think that Advanced is the place for this, but if @jameskoster and @richtabor above both agree on a new panel, I'll defer to them.

@Poliuk
Copy link

Poliuk commented Aug 29, 2023

I personally still think that Advanced is the place for this

The problem with adding it under advanced is that we won't have access to "Reset to default", and as Artemio explained above we need that to deal with the inheritance and also allow users to reset the block override.. Otherwise the toggle won't be enough since in addition to the "Disabled" and "enabled" states, we need the user to be able to reset the Lightbox to default.

Doesn’t fit well in the styles tab as it’s not a style at all, but a setting.

Adding it as a new setting would also work. And won't prevent the need of adding a dedicated panel for the Lightbox.

@jasmussen would this solution work for you? Looks like it will for @richtabor

@jameskoster
Copy link
Contributor

Adding it as a new setting would also work

I think this is the way to go. A 'Behaviors' panel seems likely to exist in the future, but I'm not yet 100% convinced that lightbox would live there because it is so specific to images.

My only remaining question is whether or not to include the "Lightbox" label alongside the more verbose "Expand on click" to cover all bases. I still think a more technical user will be scanning the UI for that term.

lightbox

@Poliuk
Copy link

Poliuk commented Aug 29, 2023

Adding it as a new setting would also work

I think this is the way to go

Great! I'd like @artemiomorales to confirm that this change fits all the needs we discussed yesterday, but I think we have it.

My only remaining question is whether or not to include the "Lightbox" label

Yes, let's keep it. I agree with you, that it will help to the more technical users. And for those who are not familiar with the term, we have the "expand on click".

@jasmussen
Copy link
Contributor

That latest mockup feels to me like a great i1, nice work!

@michalczaplinski
Copy link
Contributor

Adding it as a new setting would also work

I think this is the way to go

I think that would work, but does that entail adding a new "Settings" panel in the Global Styles for the Image block?

This is what I referred to in my earlier comment with "However, in the Global Styles, there is no "Settings" panel. There is only a "Styles" panel".

@richtabor
Copy link
Member

My only remaining question is whether or not to include the "Lightbox" label alongside the more verbose "Expand on click" to cover all bases. I still think a more technical user will be scanning the UI for that term.

"Expand on click" feels better solo.

I don't think a double-label is necessary. I wouldn't expect technical/advanced users to trip up here.

@richtabor
Copy link
Member

I think that would work, but does that entail adding a new "Settings" panel in the Global Styles for the Image block?

I think that's fine.

I think this is the way to go. A 'Behaviors' panel seems likely to exist in the future, but I'm not yet 100% convinced that lightbox would live there because it is so specific to images.

Agreed. The Aspect Ratio ToolsPanelItem works like this for reference.

@Poliuk
Copy link

Poliuk commented Aug 29, 2023

"Expand on click" feels better solo.

I don't think a double-label is necessary. I wouldn't expect technical/advanced users to trip up here

Okay, I've added a note in the issue where we were discussing the naming

@artemiomorales
Copy link
Contributor Author

@jameskoster @richtabor Ok to clarify, this would be the design in the global styles, right?

global-styles-lightbox-design-v2

And this would be the design in the block settings?

block-settings-lightbox-design-v3

Please confirm if this is the approach we can take and if so, @michalczaplinski and I will continue working on the implementation 😄

@jameskoster
Copy link
Contributor

@artemiomorales yes, that looks right :)

@richtabor richtabor moved this to 🏗️ In Progress in WordPress 6.4 Editor Tasks Aug 29, 2023
@jordesign jordesign added the [Type] Enhancement A suggestion for improvement. label Aug 30, 2023
@artemiomorales artemiomorales added [Status] In Progress Tracking issues with work in progress and removed Needs Design Needs design efforts. labels Aug 30, 2023
@artemiomorales
Copy link
Contributor Author

@jameskoster @jasmussen

Do we want to add Push to Global Styles for the lightbox setting?

I'm not sure it makes sense for the lightbox and am leaning towards no. Notably, users cannot push the duotone to the Global Styles:

duotone-push-to-global.mp4

@SaxonF
Copy link
Contributor

SaxonF commented Sep 3, 2023

I'm not sure it makes sense for the lightbox and am leaning towards no.

I'd +1 this

@artemiomorales
Copy link
Contributor Author

artemiomorales commented Sep 6, 2023

A heads up that we've received feedback from @youknowriad on the design of the UI.

Right now how we've implemented it is that the block setting inherits from the global styles and theme.json, and the global styles inherits from the theme.json, and while the value can be overridden, the inheritance isn't shown explicitly in the UI.

Here are Riad's concerns in more detail:

The reason I'm insisting here is because this setting is going to create a precedent both in terms of UI and behavior, it's the first "block attribute" that can be enabled globally in theme.json and modified locally as well. (I think layout is similar but its UI is probably not the best).

Also, in global styles, we already have several issues that were raised because of this: in global styles, in most UI, we have "reset" links to reset to the theme.json provided values but we don't have the UI to explicitely reset styles to "undefined". (We need to solve that at some point and your work here is going to help us I believe).

I mention this here to give visibility on this and allow other folks to weigh in, thanks! 🙏

@jameskoster
Copy link
Contributor

This problem is essentially #49278, no?

I think the designs in #49278 (comment) should work for this, but I don't know that it's something we'd implement for a single control?

@michalczaplinski
Copy link
Contributor

This problem is essentially #49278, no?

That's right!

This problem is essentially #49278, no?

I think the designs in #49278 (comment) should work for this, but I'm also skeptical of implementing this for a single control. I also anticipate some back-and-forth on the details, which might make it hard to ship in time for 6.4.

I would prefer to ship the design we already have for 6.4, and we can work on the updated controls for the next release. What do you think?

cc @youknowriad

@youknowriad
Copy link
Contributor

@michalczaplinski Fine by me 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Feature] Interactivity API API to add frontend interactivity to blocks. [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.