-
Notifications
You must be signed in to change notification settings - Fork 4.7k
[RNMobile] Add "Set as Featured Image" Button to Image Block (Android Only) #28854
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
Closed
SiobhyB
wants to merge
72
commits into
WordPress:trunk
from
SiobhyB:update/featured-image-button-in-image-block-settings
Closed
Changes from 1 commit
Commits
Show all changes
72 commits
Select commit
Hold shift + click to select a range
a4a52fe
Add "set as featured image" button to image block
SiobhyB 4040201
Add "setFeaturedImage" function in bridge code
SiobhyB 6bcfbd9
Add setFeaturedImage function to MainApplication
SiobhyB 0cff0d7
Add "OnSetFeaturedImageListener" listener
SiobhyB dbec57d
Update edit.native.js with small syntax correction
SiobhyB d66bd01
Set up "OnSetFeaturedImageListener" in WPAndroidGlue.java
SiobhyB d1ed155
Passing featured image ID from native (Android) to Gutenberg
SiobhyB 6a57885
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 444f3e3
Restore missing curly brace
SiobhyB 2cbe570
Updates to featured-banner.native.js
SiobhyB 073fa92
Attempts to select featured image using getEditedPostAttribute()
SiobhyB 64359b1
Update featured-banner.native.js
SiobhyB 48f8ea1
Add featured banner to image/index.native.js
SiobhyB f114ade
Update featured image banner and button styles
SiobhyB 46ba7e8
Create getFeaturedImageId function and cleanup
SiobhyB b8f05f4
Tidy up edit.native.js
SiobhyB c7d00f3
Update AndroidGlueCode
SiobhyB 84a8850
retrigger checks
SiobhyB eb78dc4
Update setup.js
SiobhyB bed517e
retrigger checks
SiobhyB 64b1c8b
Update function name from "featuredImageIdChange" to "featuredImageId…
SiobhyB d8c0824
Rename "featuredImageIdNotifier" to "onRequestFeaturedImageId"
SiobhyB 32f134d
Create OnSetFeaturedImageListener
SiobhyB 75de59f
Dismiss bottomsheet when "setFeaturedImage" is called
SiobhyB 67369d3
Introduce "onRemoveFeatured" button and tidying up names
SiobhyB c9aba0a
Updates to function names
SiobhyB a9d051f
Convert FeaturedBanner to Badge component
SiobhyB a92525b
Update edit.native.js to correct merge conflict
SiobhyB 54a5f14
Correct merge conflict with GutenbergBridgeJS2Parent.java
SiobhyB e99999c
retrigger checks
SiobhyB be31d63
retrigger checks
SiobhyB 9795874
retrigger checks
SiobhyB 2284524
retrigger checks
SiobhyB 2736f33
Update edit.native.js to fix merge conflict
SiobhyB e9ebc69
Update edit.native.js to check if an image is featured uppn changes
SiobhyB 2eb1979
Update styles.native.scss to add border to the top of featured button
SiobhyB a33b8b9
Update edit.native.js
SiobhyB 0cdeadb
Introduce "featuredMedia" prop for use when editor loads
SiobhyB 737627d
Fetch ID of post's initial featured image
SiobhyB 9e524ea
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 6d53341
Updated featured state when image is replaced within block
SiobhyB cc00dc2
Remove checkIfFeaturedImage function
SiobhyB 4dc6cef
Fix merge conflict
SiobhyB b335086
Fix merge conflict
SiobhyB 3cbe30a
Add "setFeaturedImage" and "featuredImageIdCurrent" functions to Swif…
SiobhyB 39303e4
Simplify this.state.isFeaturedImage by converting to a boolean inside…
SiobhyB 0c69a62
Comment out iOS-specific changes to the bridge
SiobhyB cc4aa8d
Add iOS functions to bridge
SiobhyB d71d067
Flag featured image when editor mounts and comment out iOS methods fr…
SiobhyB 28f19af
Comment out a bridge function for iOS (missing from previous commit).
SiobhyB 262291f
Add set featured image functionality behind dev flag
SiobhyB a17e6e9
Move "set as featured" button behind devOnly and androidOnly flag
SiobhyB 1c4b2ca
Calling "__DEV__" directly instead of defining a separate variable
SiobhyB ba7c283
Update changelog.md
SiobhyB 206fede
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 1f844b6
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 9761c04
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 85844e4
Remove "subscribeFeaturedImageIdCurrent" functionality
SiobhyB 1f61eb8
Remove featuredImageId check in componentDidMount()
SiobhyB 35fa071
Remove isFeaturedImage functionality from image/index.native.js
SiobhyB 590040b
Remove iOS functions from bridge
SiobhyB 619f4f7
Remove android.util.Log import
SiobhyB dd6a63f
Remove redundant space
SiobhyB 35f7023
Update CHANGELOG.md
SiobhyB 2ed72bf
Tweak androidOnly const
SiobhyB c74e737
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 68ef75e
Refactor 'set as featured' button into its own function
SiobhyB fbe83c2
Merge branch 'update/featured-image-button-in-image-block-settings' o…
SiobhyB e3ba262
Merge branch 'trunk' into update/featured-image-button-in-image-block…
SiobhyB 9caa86a
Fix syntax issue with onSetFeatured function
SiobhyB fe2c108
Merge remote-tracking branch 'upstream/trunk' into update/featured-im…
SiobhyB 89a8c6c
Merge branch 'WordPress:trunk' into update/featured-image-button-in-i…
SiobhyB File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Update function name from "featuredImageIdChange" to "featuredImageId…
…Current" With this commit, I'm updating a function's name from "featuredImageIdChange" to "featuredImageIdCurrent" in order to more accurately reflect its purpose.
- Loading branch information
commit 64b1c8bafa0bde962b37dd28ae6fdad2097087ac
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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:onFeaturedImageIdChangeto make it read like an event handler? The originalfeaturedImageIdNotifiermakes 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
featuredImageIdNotifiertoonRequestFeaturedImageIdin 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:
onRequestFeaturedImageIdonly 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 originalgetFeaturedImageIdis 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
onGetFeaturedImageIddoesn'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
getFeaturedImageIdfunction is only checking whether an image is featured, I've changed its name tocheckIfFeaturedImagefor the time being.To further clarify: The
checkIfFeaturedImagefunction 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
getEditedPostAttributeto 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
setAttributesto get this working in ComponentDidUpdate here. How does this seem? Looking forward to any feedback or ideas on this one. :)Uh oh!
There was an error while loading. Please reload this page.
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!
Uh oh!
There was an error while loading. Please reload this page.
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!
Uh oh!
There was an error while loading. Please reload this page.
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
setFeaturedImageStateto anisFeaturedImageboolean. 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
isFeaturedImageis 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.