Make WordPress Core

Opened 16 years ago

Closed 16 years ago

Last modified 2 months ago

#5609 closed enhancement (fixed)

New image uploader

Reported by: tellyworth's profile tellyworth Owned by:
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Here's a patch for a new image uploader. The visual design is to spec for the new UI. It's one portion of the new media library popup for the post edit page.

Uploading and sending to the editor happens in a single step: clicking the Add Image button does the upload, inserts the attachment post, sends the img src html to the editor, and closes the popup.

The popup is handled by Thickbox, with the contents in an iframe. I made some wp-specific changes to the Thickbox js: $() changed to jQuery(); addLoadEvent() instead of read() (thanks mdawaffe); and the close link changed to a [x] png as per the design spec.

I've attempted to split the code into functions that are specific to the image uploader (functions named image_*()) and functions that will be useful for other media uploaders (video, audio) - though they'll probably need some tweaking. There's a trivial hook for other media uploaders in the form of the media_upload_{$type} action in wp-admin/media-upload.php.

Also yet to be implemented are the other tabs on the image uploader ("Media Library" and "Flickr"). They're just hard-coded for now. I considered a flexible tab system like the old $wp_upload_tabs thing, but I think it'd be better to go with something simpler instead, like a simple action.

All the old uploader code is still in place for now, but should be removed once this is solid enough.

It would be possible to do this using Ajax instead of an iframe but the iframe approach seems to offer a better fallback and more reusable code.

Attachments (7)

loadingAnimation.gif (5.7 KB) - added by tellyworth 16 years ago.
wp-includes/js/thickbox/loadingAnimation.gif
tb-close.png (506 bytes) - added by tellyworth 16 years ago.
wp-includes/js/thickbox/tb-close.png
align-center.png (571 bytes) - added by tellyworth 16 years ago.
wp-admin/images/align-center.png
align-left.png (587 bytes) - added by tellyworth 16 years ago.
wp-admin/images/align-left.png
align-none.png (453 bytes) - added by tellyworth 16 years ago.
wp-admin/images/align-none.png
align-right.png (556 bytes) - added by tellyworth 16 years ago.
wp-admin/images/align-right.png
media-upload-r6577.patch (34.2 KB) - added by tellyworth 16 years ago.
correct patch file this time

Download all attachments as: .zip

Change History (28)

@tellyworth
16 years ago

wp-includes/js/thickbox/loadingAnimation.gif

@tellyworth
16 years ago

wp-includes/js/thickbox/tb-close.png

@tellyworth
16 years ago

wp-admin/images/align-center.png

@tellyworth
16 years ago

wp-admin/images/align-left.png

@tellyworth
16 years ago

wp-admin/images/align-none.png

@tellyworth
16 years ago

wp-admin/images/align-right.png

#1 @tellyworth
16 years ago

Images attached separately as the patch file doesn't include them dammit.

@tellyworth
16 years ago

correct patch file this time

#2 @ryan
16 years ago

(In [6579]) New image uploader from tellyworth. see #5609

#3 @nbachiyski
16 years ago

If the user has disabled the visual editor from her profile, the Add media: bar isn't shown.

#4 @lloydbudd
16 years ago

  • Milestone changed from 2.6 to 2.5

#5 in reply to: ↑ description @janbrasna
16 years ago

Replying to tellyworth:

"Uploading and sending to the editor happens in a single step: clicking the Add Image button does the upload, inserts the attachment post, sends the img src html to the editor, and closes the popup."

The functions image_send_to_editor() and media_send_to_editor() should be somehow pluggable to allow a) inserting the images in e.g. Textile formatting or with Lightbox and similar additions; b) inserting other media as Flash/FLV in a specific way (think SWFObject, UFO or vPIP); instead of the fixed HTML code for that image/medium.

#6 @lloydbudd
16 years ago

Relates to #5722.

#7 @andy
16 years ago

  • Resolution set to fixed
  • Status changed from new to closed

This ticket was mentioned in PR #5835 on WordPress/wordpress-develop by @Bernhard Reiter.


3 months ago
#8

  • Keywords has-unit-tests added

WIP. Pulls together code from #5609 (Extract insert_hooked_blocks() function

Trac ticket:

@Bernhard Reiter commented on PR #5835:


3 months ago
#9

## Advantages of this approach

  • Neatly separate from hooked_block_types, reasonable separation of concerns: The former allows modifying _which_ block types should be hooked, the latter is for more granular control -- e.g. to set attributes (and has access to anchor block’s).
  • Using the hooked_block_{$hooked_block_type} nomenclature would be somewhat familiar from e.g. `render_block_{$type}`.

## Downsides of this approach
The major downside is that while it might be tempting to use this filter to use a pattern instead of a hooked block, it's not a good idea to do so.

If we allow attributes, people might start adding a core/pattern block, and specify the pattern's slug via the slug attribute. However, it is still required to add the core/pattern block as a hooked block in the first place. Since third-party blocks cannot modify a Core block's block.json (and shouldn't resort to doing so via the [https://developer.wordpress.org/block-editor/reference-guides/filters/block-filters/#registration block_type_metadata or block_type_metadata_settings filters]), this would need to be done via the `hooked_block_types` filter.

For example: A Like button block uses the hooked_block_types filter to add a core/pattern block after the Post Content block, and then uses the hooked_block filter to set its slug attribute to my/like-button-wrapper.

But what if a Subscribe button block does the exact same, except setting the pattern slug to third-party/subscribe-button-wrapper?

Both blocks might assume at hooked_block filter that the core/pattern block that was added after core/post-content is "theirs"; consequently, they will both try to set the Pattern block's slug attribute. As a result, we'll likely end up with only _one_ hooked Pattern block -- and it's determined by filter order which block pattern prevails. Not ideal 😕

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks. (See https://github.com/WordPress/wordpress-develop/pull/5810 for a somewhat related PR that seeks to warn against "incompatible" _anchor_ blocks.) Then again, maybe this is not so clear-cut: After all, it makes sense to e.g. use Core's Login/out block as a hooked block that's the last child of the Navigation block.

To offer alternatives for extenders, we might want to allow passing inner blocks to the filter; see.

@Bernhard Reiter commented on PR #5835:


3 months ago
#10

To offer alternatives for extenders, we might want to allow passing inner blocks to the filter; see.

Noting that there might be a bit of an inconsistency with this when used to solve https://core.trac.wordpress.org/ticket/60126, as e.g. the hooked_block filter -- when called for the Like button block -- would return a Group block (which has the Like button block as its inner block). Hopefully not a dealbreaker though 🤔

@Bernhard Reiter commented on PR #5835:


3 months ago
#11

Here's a proof-of-concept (based on the example from #5837) that works with this PR to replace the Like Button block with a pattern that wraps it in a Group block:

  • like-button.php

    diff --git a/like-button.php b/like-button.php
    index 65acbc3..4728ce1 100644
    a b  
    2222 */
    2323function create_block_like_button_block_init() {
    2424       register_block_type( __DIR__ . '/build' );
     25
     26       register_block_pattern(
     27               'ockham/like-button-wrapper',
     28               array(
     29                       'title'       => __( 'Like Button', 'like-button' ),
     30                       'description' => _x( 'A button to like content.', 'Block pattern description', 'like-button' ),
     31                       'content'     => '<div class="wp-block-group"></div>',
     32                       'inserter'    => false
     33               )
     34       );
    2535}
    2636add_action( 'init', 'create_block_like_button_block_init' );
     37
     38function insert_like_button_pattern_after_post_content( $hooked_block, $position, $anchor_block ) {
     39       if ( 'before' !== $position && 'after' !== $position ) {
     40               return $hooked_block;
     41       }
     42
     43       if ( 'core/post-content' !== $anchor_block['blockName'] ) {
     44               return $hooked_block;
     45       }
     46
     47       // We replace the "simple" block with a pattern that contains a Group block wrapper.
     48       return array(
     49               'blockName' => 'core/pattern',
     50               'attrs'     => array(
     51                       'slug' => 'ockham/like-button-wrapper',
     52               ),
     53       );
     54}
     55add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_pattern_after_post_content', 10, 3 );

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

@Bernhard Reiter commented on PR #5835:


3 months ago
#12

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

@Bernhard Reiter commented on PR #5835:


3 months ago
#13

Per https://github.com/WordPress/wordpress-develop/pull/5835/commits/b34e2cf374d4acb128f2e63ed70de3282b9cc6ff (which I just worked on with @c4rl0sbr4v0), it is now possible to pass a block that has inner blocks:

  • like-button.php

    diff --git a/like-button.php b/like-button.php
    index 65acbc3..2c66824 100644
    a b function create_block_like_button_block_init() { 
    2424       register_block_type( __DIR__ . '/build' );
    2525}
    2626add_action( 'init', 'create_block_like_button_block_init' );
     27
     28function insert_like_button_after_post_content( $hooked_block, $position, $anchor_block ) {
     29       if ( 'before' !== $position && 'after' !== $position ) {
     30               return $hooked_block;
     31       }
     32
     33       if ( 'core/post-content' !== $anchor_block['blockName'] ) {
     34               return $hooked_block;
     35       }
     36
     37       $attrs = isset( $anchor_block['attrs']['layout']['type'] )
     38               ? array(
     39                       'layout' => array(
     40                               'type' => $anchor_block['attrs']['layout']['type']
     41                       )
     42               )
     43               : array();
     44
     45       // Wrap the Like Button block in a Group block.
     46       return array(
     47               'blockName' => 'core/group',
     48               'attrs'     => $attrs,
     49               'innerBlocks' => array(
     50                       array(
     51                               'blockName'    => 'ockham/like-button',
     52                               'attrs'        => array(),
     53                               'innerBlocks'  => array(),
     54                               'innerContent' => array(),
     55                       ),
     56               ),
     57               'innerContent' => array(
     58                       '<div class="wp-block-group">',
     59                       null,
     60                       '</div>'
     61               ),
     62       );
     63}
     64add_filter( 'hooked_block_ockham/like-button', 'insert_like_button_after_post_content', 10, 3 );

It's not exactly pretty though 😬

@Bernhard Reiter commented on PR #5835:


2 months ago
#14

I've absorbed this comment and this one into the PR description to see at a glance how the new filter can be used with different techniques.

@Bernhard Reiter commented on PR #5835:


2 months ago
#15

This seems reasonably ergonomic. The major downside is still that it doesn't allow setting the layout attribute dynamically (and instead hard-wires it to be "type": "constrained").

I wonder if pattern overrides (formerly known as partial sync patterns) would allow to solve this problem. I'll need to research that.

I've talked to folks who are working on pattern overrides, and it seems like they only work for a block's content, not for arbitrary attributes.

@Bernhard Reiter commented on PR #5835:


2 months ago
#16

@gziolo suggested to allow returning null from the filter, to indicate that the hooked block shouldn't be rendered after all.

I decided against this. The reason is that we already have the hooked_block_types filter to add and remove a block from the list of hooked blocks; and while returning null from the hooked_block filter would allow us to remove it, the opposite -- adding a block via that filter -- is not possible (lack of symmetry).

Furthermore, by not allowing removal of a block via the hooked_block filter, we retain the invariant that that filter will not change the list of hooked blocks, which should make code overall more predictable and resilient. (It avoids e.g. problems if one hooked_block filter returns null, and the next one tries to access and/or modify that block.)

@Bernhard Reiter commented on PR #5835:


2 months ago
#17

Opening for review. Reviewers, please take the time to read the PR description, which I've updated with the rationale behind this filter, plus some code snippets that demonstrate how to use it 🙌

@Bernhard Reiter commented on PR #5835:


2 months ago
#18

FYI @tjcafferkey @nerrad @yansern @TimBroddin 🙂 Feel free to review!

@tomjcafferkey commented on PR #5835:


2 months ago
#20

From my brief time working with and understanding this API, this looks like an acceptable solution to me, I like how your solution is relatively 'simple' and readable and solves this particular problem. I do have a few comments though:

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

Also, the API feels quite flexible based on the use cases you've presented in the PR description. If we release something which can be used in a multitude of ways would it limit our ability to safely iterate on and improve it going forward if we need to consider so many use-cases? Just a thought.

@Bernhard Reiter commented on PR #5835:


2 months ago
#22

Thanks a lot for your feedback @tjcafferkey, it's much appreciated!

From my brief time working with and understanding this API, this looks like an acceptable solution to me. I like how your solution is relatively 'simple' and readable yet solves this particular problem. I do have a few comments though:

To avoid this from happening, we might even consider warning (or banning) Core blocks from being used as hooked blocks.

I'd be against this for the reason you've outlined (using the core/loginout block) especially as Gutenberg's block library expands this will become more of a problem. It feels unharmonious to release an API in core whilst at the same time be trying to encourage people to use Gutenberg but not allow one to work with the other.

Yeah, that's a fair point; I agree now that we shouldn't wholesale ban Core blocks 👍

Also, the API feels quite flexible based on the use cases you've presented in the PR description. If we release something which can be used in a multitude of ways would it limit our ability to safely iterate on and improve it going forward if we need to consider so many use-cases? Just a thought.

Yes, the technique we're about to explore in https://github.com/WordPress/gutenberg/pull/57754 might work with similar use cases (i.e. blocks that fetch data from a separate entity in the DB) 👍 Off the top of my head, it might carry over quite well to core/template-part; it might be a bit trickier with core/post-content (where it would end up injecting hooked blocks into the actual post, which would mean we'd actually start injecting into content, and those hooked blocks would appear in the post editor; this would be kind of a major decision to make).

The thing is that by default, firstChild/lastChild insertion only works with container blocks (i.e. blocks that support having inner blocks), which the Navigation, Template Part, and Post Content block aren't, which is why we need to manually add support. I still think there could be some benefit in showing a warning when people attempt to insert into something like that, as I've seen a number of folks try that, and it left them confused when it didn't work. I'd love for them to have a shorter feedback loop to understand why that's happening (and to not give up on using Block Hooks 😅), and it seems easy enough to implement a warning like that.

OTOH I agree that we might continue to add support for some of these blocks in GB, and we don't want to discourage people forever from using them. Maybe we need a filterable list of unsupported blocks that the warning checks against 😅

Note: See TracTickets for help on using tickets.