#5609 closed enhancement (fixed)
New image uploader
Reported by: |
|
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)
Change History (28)
#3
@
17 years ago
If the user has disabled the visual editor from her profile, the Add media:
bar isn't shown.
#5
in reply to:
↑ description
@
17 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.
This ticket was mentioned in PR #5835 on WordPress/wordpress-develop by @Bernhard Reiter.
14 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:
14 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:
14 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:
13 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 22 22 */ 23 23 function create_block_like_button_block_init() { 24 24 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 ); 25 35 } 26 36 add_action( 'init', 'create_block_like_button_block_init' ); 37 38 function 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 } 55 add_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:
13 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:
13 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() { 24 24 register_block_type( __DIR__ . '/build' ); 25 25 } 26 26 add_action( 'init', 'create_block_like_button_block_init' ); 27 28 function 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 } 64 add_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:
13 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:
13 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:
13 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:
13 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:
13 months ago
#18
FYI @tjcafferkey @nerrad @yansern @TimBroddin 🙂 Feel free to review!
@tomjcafferkey commented on PR #5835:
13 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:
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:
13 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:
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 😅
@Bernhard Reiter commented on PR #5835:
13 months ago
#23
Committed to Core in https://core.trac.wordpress.org/changeset/57354.
wp-includes/js/thickbox/loadingAnimation.gif