Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RNMobile] Add "Set as Featured Image" Button to Image Block (Android Only) #28854
[RNMobile] Add "Set as Featured Image" Button to Image Block (Android Only) #28854
Changes from 16 commits
a4a52fe
4040201
6bcfbd9
0cff0d7
dbec57d
d66bd01
d1ed155
6a57885
444f3e3
2cbe570
073fa92
64359b1
48f8ea1
f114ade
46ba7e8
b8f05f4
c7d00f3
84a8850
eb78dc4
bed517e
64b1c8b
d8c0824
32f134d
75de59f
67369d3
c9aba0a
a9d051f
a92525b
54a5f14
e99999c
be31d63
9795874
2284524
2736f33
e9ebc69
2eb1979
a33b8b9
0cdeadb
737627d
9e524ea
6d53341
cc00dc2
4dc6cef
b335086
3cbe30a
39303e4
0c69a62
cc4aa8d
d71d067
28f19af
262291f
a17e6e9
1c4b2ca
ba7c283
206fede
1f844b6
9761c04
85844e4
1f61eb8
35fa071
590040b
619f4f7
dd6a63f
35f7023
2ed72bf
c74e737
68ef75e
fbe83c2
e3ba262
9caa86a
fe2c108
89a8c6c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Similar to my naming comment above, what do you think about renaming to
featuredImageIdChanged()
or better:onFeaturedImageIdChange
to make it read like an event handler? The originalfeaturedImageIdNotifier
makes me wonder what gets notified... like, is it the user that receives a notification when this function is called?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.
Reading more code where this function is used, I see that it's also called when the JS side just asks for an update of the featured image id. So, the function's purpose is not only to notify the JS side that the id has changed, which means that the event name (defined here) is not actually accurate 🤔.
So, if I understand things properly, the event is all in all just trying to pass the the media id of the current featured image, right, whether it was recently updated or not? If so, what do you think about naming the event with
EVENT_FEATURED_IMAGE_ID_CURRENT = "featuredImageIdCurrent"
?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 useful feedback, thank you @hypest! I looked through other similar functions and renamed
featuredImageIdNotifier
toonRequestFeaturedImageId
in order to hopefully be clearer of the function's purpose. Let me know if you think this is still unclear!That makes sense! The purpose changed as the code evolved (it was originally only passing the id when the image changed) which is why I think the naming became a bit messy/confusing. This is a good lesson to keep on top and iterating on names. Thank you! I've updated this to "featuredImageIdCurrent" now.
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.
Aha.
Still trying to grok what the code is trying to do, please bear with me :). There are two rather different code paths that use this function:
onRequestFeaturedImageId
only if the media id provided matches the featured image id on file. By the way, looks like it's not a getter after all since the response to the originalgetFeaturedImageId
is conditional. Looks to me more like a "check for something", not a plain getter. If so, would it make sense to rename getFeaturedImageId -> checkIfFeaturedImage?From flow No2 above, and the function implementation itself (native side,
gutenberg/packages/block-library/src/image/edit.native.js
Lines 305 to 319 in 75de59f
sendToJSFeaturedImageId
?Then, flow No1 shows to me we need to better name and document why
onGetFeaturedImageId
doesn't always call sendToJSFeaturedImageId.Does this all make sense? Sorry if I'm a bit confused on this one.
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.
@hypest, thank you again for your help here!
It's right that the
getFeaturedImageId
function is only checking whether an image is featured, I've changed its name tocheckIfFeaturedImage
for the time being.To further clarify: The
checkIfFeaturedImage
function is called withincomponentWillUnmount()
on the JS side. It's currently responsible for flagging which image is featured when the editor is first loaded.The background for this is that I was unable to get
getEditedPostAttribute
to work to get the post's initial featured image (discussed on project thread and in project's Slack channel).I've spent a bit of time with this but am not sure if there's a more elegant way to achieve the same effect, but it's entirely likely I've been looking at the code too long! I'd be interested to hear whether you think this approach makes sense, with those extra details, or if I should work on this a bit more.
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.
I've been able to get this working to grab the post's initial featured image ID now and have made use of
setAttributes
to get this working in ComponentDidUpdate here. How does this seem? Looking forward to any feedback or ideas on this one. :)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.
Yeah, I think we're on a good route here. We chatted a bit over Zoom, to share my thoughts on how the ImageEdit component just needs to listen to the store updates and compare the featured image id it gets from there with the id it keeps in the block attributes, and use those to compute a "isFeaturedImage" boolean inside the render() function.
Then, it becomes a matter of figuring out how to also do the update of the id in the store, for which we can use the editEntityRecord() action creator (acquiring it via the
withDispach
. But, that part (to update the featured image id value when the image changes) can be done in a separate PR I think.Let me know what you think!
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.
Thank you again for this! I've spent a bit of time today trying to process this and have a few questions:
setFeaturedImageState()
functionality inside therender()
function vs. the current approach?setFeaturedImageState()
is currently called incomponentDidMount()
for when the editor first loads and thencomponentDidUpdate()
for when an image is replaced directly within the image block (using the Edit icon to the upper right when the block is selected). Will moving the functionality for setting state fromsetFeaturedImageState()
torender()
resolve the need to do that? Is that one of the benefits?editEntityRecord()
to grab the post's current featured image ID before I can move the functionality for setting state fromsetFeaturedImageState()
. Is that right?Sorry for my fuzziness on this one, just want to make sure I'm clear before moving ahead!
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 seems all I had to do was ask those questions and then play around a little more to figure out the answers. :) I've been able to convert the functionality previously included in
setFeaturedImageState
to anisFeaturedImage
boolean. I can see now that the answer to my second question is "yes" and the code is a lot cleaner as a result. 🎉 I've committed those changes.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.
Nice to see the simplification of
isFeaturedImage
!Just a quick comment on the terminology used in the commit message: now that
isFeaturedImage
is just a helper local const inrender()
, referring to it as "state" can be misleading, mainly because within React, "state" has a special and specific meaning. I'd recommend referring to it as "local variable/const" to avoid confusion with React's use.