Add a "Set as Featured Image" Button to Image Block #14052
Conversation
SiobhyB
added
[Type] Enhancement
[Status] In Progress
Gutenberg
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
This commit is mostly for testing purposes, I've updated GutenbergEditorFragment.java with a function called "onSetFeaturedImageButtonClicked" and want to test the results.
This commit updated reference to latest gutenberg-mobile commit.
This commit introduces code that sets a featured image when the "set as featured" button is clicked from an image block. A function within "EditPostSettingsFragment" is currently utilised for this purpose, though this is likely not the optimal approach.
SiobhyB
force-pushed
the
update/image-block-with-featured-image-setting
branch
from
2850cff
to
b251e4b
Compare
This commit cleans up some of the changes that have been made to the GutenbergEditorFragment file, deleting unused/commented out code, etc.
Fix some issues with spacing
|
The PR is still in draft mode because, though the code technically works to set a featured image, the approach I’ve taken feels fragile/error-prone. I’ve added a new By calling the function in this way, our data tools won't distinguish between any clicks to the image block's new featured image button and the button within the Post Settings screen. It also feels fragile, like any issues with EditPostSettingsFragment may then lead to issues with the image block's button. I'm looking to explore other patterns/ways to set the featured image from GutenbergEditorFragment. cc-ing @hypest (who noted he'd be able to take a look over this later in the week) and @jd-alexander (who has familiarity with the code from buddying me with it these past weeks). Thanks in advance for your thoughts and any pointers! |
|
Looks like the bundling job fails with |
...ressEditor/src/main/java/org/wordpress/android/editor/gutenberg/GutenbergEditorFragment.java
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void featuredImageSet(int mediaId) { | ||
| mEditorFragmentListener.updateFeaturedImage(mediaId, true); |
There was a problem hiding this comment.
Looking at the code that uses the imagePicked parameter (passed as true here), my impression is that it uses that param just to trigger a Tracks event during the media picking flow.
During the set-feature-image flow from inside an image block, I'd expect to pass false so to avoid triggering that event so, I'm not sure why the true here @SiobhyB , can you elaborate?
This also seems related to the concern you expressed in the comment.
Can you elaborate on that @SiobhyB ? Like, what could be an example of an issue with EditPostSettingsFragment that'd break the image block button? In the meantime, in case that's what you have in mind too, it is a bit awkward that the editor fragment needs to call a function in the Post Settings fragment just to update the featured image id. Feels like such a function could live in a headless "service" fragment inside the EditPostActivity instead of lying inside a UI fragment. I still think that the current PR is a good first take on implementing the set-featured-image functionality, and a cleaner approach (perhaps by extracting the shared code that sets the featured image id in the local DB) could be worked on in a follow up PR. |
This commit introduces an "OnSetFeaturedImageListener" and moves new featured image related functions to it from "OnMediaLibraryButtonListener".
…turedImageId" This commit introduces a "removeFeaturedImage" function to allow users to remove a featured image directly from the image block. It also updates the name of the "onGetFeaturedImageId" to "checkIfFeaturedImage" in order to better reflect its current purpose.
…uredImageId" This commit updates "onRequestFeaturedImageId" to "sendToJSFeaturedImageId", in order to clarify the purpose of the function.
The SnackBar is currently necessary for when a featured image is removed, so removing for now.
This commit enables tracking for the image block's two new featured image buttons ("set as featured image" and "remove as featured image").
|
👋 @SiobhyB , looks like this PR is not up-to-date with the latest from the gutenberg-mobile/Gutenebrg side PRs and won't compile correctly. Can you update it? While at it, I wonder if it might be a better strategy to test/develop by using the WPAndroid app anyway (instead of the gb-mobile demo apps), since the feature includes many changes in the bridge anyway. |
|
I've updated this PR now, thanks again for looking!
I've been developing/testing using the WPAndroid app in order to allow me to test the changes I'm making. I'm still actively working on and committing to this issue while it's in draft mode, which I think is why you caught the PR while it's out-of-date. Let me know if I'm missing something else here. Edited to add: I'm aware that there are merge conflicts here, it's on my list to look into! |
The native Snackbar is going to be replaced by the new Compact Notice on the JS side.
This commits tidies up some functions that had not yet been renamed to "sendToJSFeaturedImageId", and also sends notices to the JS side when images are set or removed as featured.
Generated by 🚫 dangerJS |
This commit removes logging I'd placed in the EditPostActivity file for debugging purposes.
…id into update/image-block-with-featured-image-setting
|
Just a quick note: we weren't exactly trying to merge this PR but got merged automatically because we merged a PR that wholly included the commits already. |
With this PR, we'll be implementing a "set as featured image" button with the purpose of making it easier for users to assign a new featured image directly from the post area.
Fixes wordpress-mobile/gutenberg-mobile#1011
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3116gutenberg: WordPress/gutenberg#28854Please refer to the Gutenberg PR as the central place for steps to tests and screenshots.
PR submission checklist:
RELEASE-NOTES.txtif necessary.