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

Gallery: figure out way to fallback to shortcode #1451

Closed
mtias opened this issue Jun 26, 2017 · 20 comments
Closed

Gallery: figure out way to fallback to shortcode #1451

mtias opened this issue Jun 26, 2017 · 20 comments
Assignees
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Block] Gallery Affects the Gallery Block - used to display groups of images [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media Needs Decision Needs a decision to be actionable or relevant [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress

Comments

@mtias
Copy link
Member

mtias commented Jun 26, 2017

There will be cases where it's important galleries just output the shortcode for compatibility. How could we expose this? A setting in "settings -> media"? An add_theme_support call? A setting in the inspector?

@mtias mtias added [Feature] Blocks Overall functionality of blocks [Type] Question Questions about the design or development of the editor. [Feature] Media Anything that impacts the experience of managing media labels Jun 26, 2017
@westonruter
Copy link
Member

An add_theme_support( 'blocks' ) call makes sense to me. This could eventually unlock additional functionality, like full-bleed alignment, layout management, and other features that perhaps cannot be reliably added without themes opting into support. There would need to be a full list of the things that a theme would need to implement when they affirm they support the blocks feature.

@mtias
Copy link
Member Author

mtias commented Jul 24, 2017

There would need to be a full list of the things that a theme would need to implement when they affirm they support the blocks feature.

This makes me think we should have specific add_theme_support or an array of configuration that includes different assertions (single column theme, content width, etc).

@jasmussen
Copy link
Contributor

So, existing content will work in the freeform block. We have a pretty cool new gallery block, with a column slider. We are probably going to want to recommend this new block going forward.

I'm not sure I fully understand in which cases it's important to just output the shortcode. Would the following flow address this issue, for example?

  1. Insert gallery block, make a "new" gallery
  2. Open inspector
  3. Click "convert to classic gallery" button in the inspector

@youknowriad
Copy link
Contributor

We do have conversion from Gallery Shortcode to a Gallery block. Is this what's this issue is about. The opposite transformation doesn't make much sense to me. We should try to improve the Gallery Block to be the default right?

@mtias
Copy link
Member Author

mtias commented Mar 6, 2018

We should try to improve the Gallery Block to be the default right?

This was in the case of plugins working with a dynamic gallery—tiled galleries, etc. It might be plugin territory.

@danielbachhuber
Copy link
Member

Related #5972

I couldn't find an issue specifically for it, but we'll need to decide on how we want to handle support for the post_gallery filter (either supporting it, or considering Gutenberg support an enhancement).

@danielbachhuber danielbachhuber added the Backwards Compatibility Issues or PRs that impact backwards compatability label May 21, 2018
@tofumatt
Copy link
Member

Sorry, I'm just getting around to this issue and I don't understand it at all. I think there's some context missing?

What are the steps to reproduce the issue here? I'm not quite sure where to start.

@danielbachhuber
Copy link
Member

@tofumatt I can give you a walkthrough if you'd like (and we can post the video here afterwards). It's a relatively nuanced issue.

@tofumatt
Copy link
Member

Sure thing! If you wanted to just post a video that works for me, but if you wanted to setup a time to meet/skype/whatever that's cool too. Just ping me on Slack if you wanted to meet for a walkthrough and we can work out a time 😄

@danielbachhuber
Copy link
Member

@tofumatt and I chatted on Zoom today. Here's a video recording of the call: https://www.dropbox.com/s/ihm9zedsd2onn7s/20180814galleryexplanation.mov?dl=0

@tofumatt may provide a summarized transcription of the call at some point in the near future.

@mtias mtias modified the milestones: Merge: Media, WordPress 5.0 Oct 3, 2018
@mtias mtias added Needs Decision Needs a decision to be actionable or relevant and removed [Type] Question Questions about the design or development of the editor. labels Oct 3, 2018
@antpb
Copy link
Contributor

antpb commented Oct 3, 2018

I finished listening to the zoom @danielbachhuber and @tofumatt provided. Thank you so much for recording that. I wanted to post a summary here so we can get some traction on discussion.

It seems the big issue with the Gallery block in terms of backwards compatibility, is that it bypasses the post_gallery filter.

Many developers build custom gallery implementations by using that. The current Gallery block is more static than the previous so we have issues when things like an undefined id gallery shortcode that produces a gallery of images associated with the post. That association is dynamic. The Gutenberg block loses that functionality.

[gallery]

When the [gallery] shortcode is converted, we create a gallery block from the ids associated at the time of converting to the block. If the user adds more attachements to the post after converting, the gallery will not update when those changes are made.

I have some thoughts that may be helpful or at the very least start of discussions toward a plan:

I’m wondering if we could have a theme support option that can fallback to ServerSideRender. This would ensure that we hit the post_gallery filter. We could communicate out to theme/plugin developers that the option is available and their way to get Gutenberg compatible would be to set the option. We can keep gallery as is and depend on the theme support to set the type. This assumes a lot, but does that sound like an option forward?

@danielbachhuber
Copy link
Member

It seems the big issue with the Gallery block in terms of backwards compatibility, is that it bypasses the post_gallery filter.

Yep, more or less (e.g. #5972)

I’m wondering if we could have a theme support option that can fallback to ServerSideRender. This would ensure that we hit the post_gallery filter. We could communicate out to theme/plugin developers that the option is available and their way to get Gutenberg compatible would be to set the option. We can keep gallery as is and depend on the theme support to set the type. This assumes a lot, but does that sound like an option forward?

This seems like it could be an option. I don't have a strong opinion about it as a solution though. I also don't have any great ideas on how we should make this decision at this point.

If we were to prioritize backwards compatibility, then it might make sense for WordPress core default to ServerSideRender, and then allow themes and plugins to provide their own enhanced galleries.

@antpb antpb added this to the WordPress 5.0 milestone Oct 3, 2018
@tofumatt tofumatt assigned mcsf and unassigned mcsf Oct 3, 2018
@tofumatt
Copy link
Member

tofumatt commented Oct 3, 2018

To be clear: I've had a bit of a look at this but didn't get far and am un-assigning myself because I don't think I'll be able to get to this before WordPress 5.0. If that changes I'll have another look, but I think it's best to find someone new to work on this.

(Thanks for looking at it again, @antpb.)

Reading through this more and understanding it... I could actually see the merits in defaulting to ServerSideRender for this at least for WordPress 5.0.

@antpb
Copy link
Contributor

antpb commented Oct 3, 2018

I think this is a 5.0 task. I am still evaluating the remaining tasks for Media. I don't want to assign/commit this to myself just yet but I think I can be of help in this.

On the chance someone comes in and can tackle this before me, I have an example of what is needed to accomplish the server side rendering bits.

With the playlist block PR you can find how I'm storing/passing the attribute of ids to the wp_playlist_shortcode function:

here's that PR: #9169

Classic Editor galleries work similarly to playlists in that it is primarily dependant on ids and the rest kinda works itself out in PHP land.

The part to be discovered is how we will conditionally do either Gutenberg Gallery Block or ServerSideRender based on add_theme_support

@antpb
Copy link
Contributor

antpb commented Oct 8, 2018

I've started work in the above PR on an approach that sets the render callback based on has_filter( 'post_gallery' ) . I'm having issues currently in registering the render callback. Would REALLY appreciate if you could have a look @danielbachhuber . The way this PR sits, it should be defaulting to render_callback on a front end render of the content. For whatever reason, it is not. Going to continue plugging away at this issue tomorrow.

@danielbachhuber
Copy link
Member

@antpb Have you gotten sign-off on the proposed implementation? I think it'd make sense to do so before you invest too much time into a pull request.

@antpb
Copy link
Contributor

antpb commented Oct 8, 2018

Happy to pause to get sign off. This was discussed a bit in the media meeting last week and it was mentioned that we could explore if post_gallery dependent ServerSideRender would be a decent option. Luckily, I didn't put too much time on this. Just wanted to get familiar with the problem and options. I'd love to get an agreement on the path forward some time this week. :)

We have three possibilities that I see going forward on this (feel free to suggest others):

  1. Make a somewhat refactored 5.0 specific Gallery block that defaults to shortcode for sake of compatibility
  2. Make the render_callback conditional based on if a post_gallery filter exists (example in PR mentioned above)
  3. Add theme option that controls the behavior of the block. The option would give two options: shortcode fallback and new Gutenberg Gallery. A dev would be able to add the theme option which would allow shortcode fallback to support the post_gallery filter.

@mtias do you have any thoughts on the above options?

@markfinst
Copy link

Proposal

Render gallery only on server side.

  • On the actual website, gallery renders how theme wants (via post_gallery filter). Majority of themes support this filter.
  • In Gutenberg Editor, a gallery is rendered just as a list of image squares. With two additional features that are currently unavailable:
    • Drag to reorder (without having to open Media Library).
    • Click on the image shows its fields (Title, Caption, Alt) in right options sidebar. If it's hard to implement - open Media modal.

Advantages:

  • It's easier for the user to edit the gallery
  • Theme or plugin developers can override the gallery, instead of creating a new block.
  • A user can now define "Alt attribute" and other fields for individual images.
    • The current implementation of the caption ("Write text..") does not actually update image attachment data. So users have to update "alt text" in one place, and "caption" in another place.

Disadvantages:

  • A gallery within Gutenberg Editor looks differently from the actual website.
  • Modifying visual options (for example "Columns") does not have any effect while in the editor

If these two are a big issue, gallery block can be displayed as squares only after user clicks (or focuses) it. Or when the user clicks on "Edit Gallery" icon.

Example mockup

123

@joyously
Copy link

Why do you keep mentioning themes and theme_support for a core shortcode?
Please don't misuse the theme_support function for something that isn't a theme thing.
Isn't it possible to have shortcodes in posts that are never edited with the new editor?

@mtias mtias added the [Block] Gallery Affects the Gallery Block - used to display groups of images label Oct 23, 2018
@antpb antpb self-assigned this Oct 29, 2018
@antpb antpb added the [Priority] High Used to indicate top priority items that need quick attention label Oct 31, 2018
@mtias mtias added the [Status] In Progress Tracking issues with work in progress label Nov 13, 2018
@antpb
Copy link
Contributor

antpb commented Nov 19, 2018

What was needed in this issue was a way to parse the ids from the Gutenberg gallery. This has been achieved in #11540 so I'll be closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Block] Gallery Affects the Gallery Block - used to display groups of images [Feature] Blocks Overall functionality of blocks [Feature] Media Anything that impacts the experience of managing media Needs Decision Needs a decision to be actionable or relevant [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

No branches or pull requests

10 participants