Editor: Refactor PostFeaturedImage component #40126
Conversation
Mamaduka
added
[Type] Enhancement
|
Size Change: +110 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
Thanks for this. Given we've reverted the status panel for now, should we maybe seek to cherry-pick the reverted commits into a new PR and integrate this? It will be useful for when #39082 gets picked back up! |
|
@jasmussen, since this is a general improvement, I'm thinking of rebasing, and we can ship this in 6.0. The uploading state issue affects the current trunk as well. |
|
Sounds great 👍 👍 — let me know how I best can help. |
Mamaduka
force-pushed
the
update/post-featured-image-component
branch
from
7e583b6 to
b321ef5
Compare
|
Rebased and fixed a few minor issues. I think this is ready for review. |
|
Even without throttling, I see the same in trunk as in this branch, any idea what I'm doing wrong? That said, it seems like the time between you drop and the spinner appears, the height of the dropzone collapses to zero making the spinner hover awkwardly above a thin line. I wonder if we should add a min-height to the dropzone. Also, some of the improvements we did make to the summary panel effort could really benefit this panel still 😅 — but not for this PR. |
|
The issue with the uploading state is more visible on slow connections. You can also try uploading large images and comparing the results with the trunk. Collapse happens when dropzone is replaced with an image tag, but the image isn't rendered yet. |
|
Yep, this is a vast improvement: |
|
I think as a follow-up, we can also display images in progress. Like we do for the Image and other media blocks. |
|
I just realized that this would be a breaking change for plugins that use the I did a quick search in the WP.org directory, and the change should affect many plugins - https://wpdirectory.net/search/01G09DKAY7BA2V543ZKBHT3NEK. We can also mention the update in Misc DevNotes. |
Mamaduka
force-pushed
the
update/post-featured-image-component
branch
from
b321ef5 to
5a44b0a
Compare
There was a problem hiding this comment.
What are we adding here that is worth the breaking change? Are we making a breaking change just for refactoring code?
If I am not missing something, the one added functionally is the improvement of the upload state.
a) Can we do it without the breaking change?
b) If we're keen in deprecating HoC props can we do it at least in a few steps?
|
To be honest, I got carried away with refactoring and only then realized that it was breaking change 😅
I thought it was okay to refactor the code to modernize it. WP core does it as well.
I'm not sure if this would be possible without changing HoC props, so it still would be a breaking change. P.S. Most plugins I check in the repo don't use these props. |
Mamaduka
force-pushed
the
update/post-featured-image-component
branch
from
5a44b0a to
3ff6883
Compare
| const mediaUpload = useSelect( ( select ) => { | ||
| return select( blockEditorStore ).getSettings().mediaUpload; | ||
| }, [] ); |
There was a problem hiding this comment.
I'm grabbing mediaUpload inside the component instead of withSelect to avoid introducing another public API prop.
|
@draganescu, I've updated the code to make it backward compatible. The only "breaking" change is that |
Unfortunately, after looking at this more it seems we shouldn't introduce any breaking change here and the reason is the existence of the filter I tried to find a work around for this and the only viable solution I come up with is if we keep the old Any thoughts? --cc @youknowriad |
|
Thanks, @ntsekouras. I'm not too fond of a hacky solution as well, but happy to make the required changes, if we all agree on the direction. P.S. We made similar changes to the |
|
I think we need to assess how many plugins are using this hook. If we manage to contact them and ask them for the change, it's easier.
I think that could be a good start and if we can wrap the existing props with "proxies" we can trigger a |
Based on WPdirectory only 13 - https://wpdirectory.net/search/01G09DKAY7BA2V543ZKBHT3NEK. I can check how many of these use the props. But there are always more for client projects and private plugins.
I was also looking into how we can leverage proxies for deprecations 😄 I Don't have anything yet, but I think this is a good idea for the future. I will update the code to use Nik's suggested method. It would be great to have a plan for this kind of situation in the future. |
|
@ntsekouras, I restored all filter props, so it's not a breaking change anymore. |
Mamaduka
force-pushed
the
update/post-featured-image-component
branch
from
7e124cc to
bf9bada
Compare
|
@draganescu, @ntsekouras, I think this is still worth landing even without complete refactoring :) |
ntsekouras
left a comment
ntsekouras
left a comment
There was a problem hiding this comment.
Really nice improvement George, thanks!
bph
added
the
Needs Dev Note
|
@bph, this is an internal non-breaking change and doesn't require a Dev Note. |
Mamaduka
removed
the
Needs Dev Note
|
Thanks @Mamaduka I'll remove it from the list |
What?
Rafactors
PostFeaturedImagecomponent.Changes in this PR:
withSelectandwithDispatchHoC with hooks.Why?
Previously it was not clear that the Featured Image started uploading.
How?
The component now sets the state when the media URL is a blob.
Testing Instructions
Screenshots or screencast
CleanShot.2022-04-07.at.11.53.13.mp4