-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix featured image modal #17410
Fix featured image modal #17410
Conversation
@@ -79,6 +108,7 @@ class MediaUpload extends Component { | |||
constructor( { | |||
allowedTypes, | |||
gallery = false, | |||
unstableFeaturedImageFlow = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outside of the scope of this ticket, but I still think it would be nice to consolidate the gallery and featuredImageFlow into a single property that is responsible for determining the library flow (i.e. Frame) that is being loaded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1979 is the related issue where I originally suggested the approach of consolidating different flows into a single prop.
@@ -111,6 +141,22 @@ class MediaUpload extends Component { | |||
this.frame.$el.addClass( modalClass ); | |||
} | |||
|
|||
if ( unstableFeaturedImageFlow ) { | |||
const featuredImageFrame = getFeaturedImageMediaFrame(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to put this code inside a function buildAndSetFeatureImageFrame as we do for the gallery frame?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely would. Will do that and update the PR.
* @return {void} | ||
*/ | ||
createStates: function createStates() { | ||
this.on( 'toolbar:create:featured-image', this.featuredImageToolbar, this ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event is essentially used to change the select image button label right?
Can't we use the frame config button prop to achieve something similar?
https://github.com/WordPress/gutenberg//blob/6ed195614f24acdbf626c099b22709d30d5a4d7b/packages/media-utils/src/components/media-upload/index.js#L128-130?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for the button to render. With the Featured Image frame in Core, we have to fire a function similar to this to allow there to be a button. Without this event, there would be an absent select button in the media modal. Here's where we have it in Core: https://github.com/WordPress/wordpress-develop/blob/master/src/js/media/views/frame/post.js#L558-L563
6ed1956
to
e701b75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @anthonyburchell, there was only a simple change pending I applied and I think we can merge this PR now.
Thank you for this improvement 👍
It seems issue repeats again. Changing Theme to default. Image block dialogue missing Filter Media as "Mine", "Uploaded to this post", "Unattached" filters Any chance to check issue again? |
Description
This is to match the backbone featured image frames expected in the editor. This allows a user to select images with filters of
mine
anduploaded to post
which was previously missing in the existing Gutenberg featured image frame.How has this been tested?
Much of the discussion that lead to this pull request can be referenced here: #10810
Issue this fixes:
#8748