Skip to content
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

Add a "Set as Featured Image" Button to Image Block #14503

Merged
merged 97 commits into from
Jun 3, 2021

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Apr 21, 2021

Partial fix for: wordpress-mobile/gutenberg-mobile#1011

gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3116
gutenberg: WordPress/gutenberg#31705

Description

This PR adds a Set as Featured button to the image block's settings, with the purpose being to make it simpler for users to set a featured image within the post's editor. Users will also be able to Remove as Featured directly from the block's settings.

A couple of notable changes to the native code have been highlighted below, with further details around the JS side highlighted in the Gutenberg PR. That PR can also be referred to as the central place for up-to-date screenshots and testing flows.

OnSetFeaturedImageListener

OnSetFeaturedImageListener listens for requests to set a featured image via an image block that are passed over the bridge. Please refer to the Gutenberg PR for more details of how this implemented from the JS side.

When a request is received from the native side, onSetFeaturedImageButtonClicked() checks for whether a featured image has already been set, in which case showFeaturedImageConfirmationDialog() is called and users are able to confirm their wish to replace the featured image or not. If there isn't already an existing featured image or the user is removing an image as featured, then the setFeaturedImage() function is called directly.

Note: The code for setting a featured image has been iterated on as part of #14521, with tracks events being added as part of that PR also. It'll be merged separately as an enhancement to this one.

GutenbergDialogFragment

A new GutenbergDialogFragment class has been created as part of this PR. This class is based on the BasicFragmentDialog class, with some key differences being the removal of the third setNeutralButton, an addition of a mediaId to the constructor, and the ability for the parent fragment to implement the positive/negative button interfaces.

GutenbergDialogFragment is called from the showFeaturedImageConfirmationDialog() function, to ensure that the dialog survives screen rotation.

The mediaId associated with the image is passed down from the onSetFeaturedImageButtonClicked() function (which in turn retrieves it from the JS side).

The functionality for when a user chooses to replace a featured image is defined in the onGutenbergDialogPositiveClicked() function and the dialog is dismissed when the cancel button is clicked, as per the onGutenbergDialogNegativeClicked function.

As outlined in #14676, there are other dialogs in the GutenbergEditorFragment file that are lost/dismissed with screen rotation. Part of the goal behind the new GutenbergDialogFragment class is to allow for easier refactoring of those other Dialogs, beginning with showCancelMediaUploadDialog and showRetryMediaUploadDialog, as those functions also accept a medaId.

Testing

Please refer to the Gutenberg PR as the central place for steps to tests and screenshots. Of note for the Android side, testing whether the replace dialog works as expected when the screen is rotated will be helpful.

Regression Notes

  1. Potential unintended areas of impact

The main unintended issues would be around the incorrect featured image being set/removed or other unintended/buggy issues with featured images.

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

The flows outlined in the Gutenberg PR were tested.

  1. What automated tests I added (or what prevented me from doing so)

N/A


PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 21, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Siobhan added 7 commits April 21, 2021 19:16
This commit adds an OnSetFeaturedImageListener() in order to fetch a request to set a featured image from the JS side.
onSetFeaturedImageButtonClicked() will be called when the option to set a featured image is clicked. It'll call showFeaturedImageConfirmationDialog(), which will be fleshed out in the next commit.
This commits adds "updateFeaturedImage" as an EditorFragmentListener method in EditPostActivity.java. The function enables an update to a post's featured image. By adding the function to "EditorFragmentAbstract.java", this can then be called from "GutenbergEditorFragment.java".
These functions serve to either set a new featured image or remove an existing featured image, using the updateFeaturedImage() function defined in the previous commit. The ID of the new featured image (zero if removed) is then sent back to the JS side of the app via sendToJSFeaturedImageId().
The logic in showFeaturedImageConfirmationDialog() is fleshed out in this commit, with removeFeaturedImage() being called if the featured image is removed, setFeaturedImage() being called by itself if a new featured image is set, and an AlertDialog if there is a replacement.
The showNotice() function is utilised to send a notice to the user depending on whether a featured image is set or removed.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 21, 2021

You can test the changes on this Pull Request by downloading the APK here.

@SiobhyB SiobhyB marked this pull request as ready for review April 27, 2021 17:12
SiobhyB and others added 3 commits May 11, 2021 22:02
With this commit, the showFeaturedImageConfirmationDialog() is refactored so that it only includes code for building the confirmation dialog.

The if/else logic for determining whether the dialog displays is moved to the onSetFeaturedImageButtonClicked() function.

The goal is to make the purpose of each function clearer.
Siobhan added 2 commits June 1, 2021 10:54
With this commit, MEDIA_ID_NO_FEATURED_IMAGE_SET is imported to EditPostActivity, so that it can be used in a conditional checking for whether a featured image is being set.
With this commit, references to 'mediaId' are updated to 'id' in the GutenbergDialogFragment.kt class. The purpose is to make the name more generic so that the class is more re-usable.
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Jun 1, 2021

@jd-alexander, thank you for this. 🙇‍♀️ I've gone ahead to implement your suggestions, let me know what you think and I'll go ahead with the merge waterfall if all looks okay to you. :)

@hypest
Copy link
Contributor

hypest commented Jun 2, 2021

👋 @SiobhyB , Joel will be on AFK for the next few weeks so, I'll go ahead and take over the last steps to finalize the review here.

I gave the PR a try locally using local builds and looks like I run into an issue with multiple gradle plugin versions detected:

Build file '/Users/stefanos/proj/a8c/android/tmp/t2/WordPress-Android/gutenberg-mobile/gutenberg/node_modules/@react-native-community/masked-view/android/build.gradle' line: 17

* What went wrong:
A problem occurred evaluating project ':android:@react-native-community_masked-view'.
> Failed to apply plugin [id 'com.android.internal.library']
   > Using multiple versions of the Android Gradle plugin in the same build is not allowed

The binary (composite) build works OK, only the local source mode encounters the issue. Perhaps some change in WPAndroid or gutenberg-mobile has introduce the mismatch, can you have a look? I don't think this PR has anything to do with the mismatch, but we might as well avoid merging as-is so we don't introduce a development flow breakage. wdyt?

Siobhan added 3 commits June 2, 2021 19:54
The title of the replace dialog is updated from "Replace?" to "Featured Image Already Set" with this commit. This brings it more inline with how titles are presented in other areas of the app (questions aren't generally used for titles). The overall goal is to improve clarity.
Following feedback, it makes sense to undo title case that's applied to notices, as they're not strictly headings/titles. This will help with consistency in how notices are capitalised throughout the rest of the apps.
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Jun 2, 2021

@hypest, thank you for the heads up!

Perhaps some change in WPAndroid or gutenberg-mobile has introduce the mismatch, can you have a look? I don't think this PR has anything to do with the mismatch, but we might as well avoid merging as-is so we don't introduce a development flow breakage. wdyt?

There was a little bit of a window between Android and Gutenberg updating to 4.0.2, which explains what happened here. I've pulled the latest changes from Gutenberg now, so all should work again. :)

@hypest
Copy link
Contributor

hypest commented Jun 3, 2021

@SiobhyB , do you perhaps get this AndroidStudio issue Cannot access 'androidx.activity.result.ActivityResultCaller' which is a supertype of 'org.wordpress.android.editor.gutenberg.GutenbergDialogFragment'. Check your module classpath for missing or conflicting dependencies? See screenshot too. WPAndroid builds fine both via cli gradle and AndroidStudio (and dialog works fine too) so, not a blocker for the PR, just wondering if it's on my side alone or not.

Screenshot 2021-06-03 at 11 27 50

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this merged @SiobhyB !

I think the libraries misalignment error AndroidStudio is giving out is not a blocker but, can you please open a ticket and check it out in a separate PR? WDYT?

@SiobhyB SiobhyB added this to the 17.6 milestone Jun 3, 2021
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Jun 3, 2021

Thank you for this, @hypest! I'm going to begin the domino. :)

Just noting our Slack conversation, also, for posterity: p1622711015069900/1622706783.069600-slack-C01NFTM4BMY

Although I'm not currently able to replicate the error you're seeing (likely due to an older version of AS), I'll look further into this and create a GH issue. ✅

private val enableAudioBlock: Boolean,
private val isAudioBlockMediaUploadEnabled: Boolean,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes come from this PR, that it was required to merge it here due to conflicts in the Gutenberg reference.

enableAudioBlock = enableAudioBlock,
isAudioBlockMediaUploadEnabled = isAudioBlockMediaUploadEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes come from this PR, that it was required to merge it here due to conflicts in the Gutenberg reference.

@SiobhyB SiobhyB merged commit 2814689 into develop Jun 3, 2021
@SiobhyB SiobhyB deleted the gb/1011-set-featured-button branch June 3, 2021 15:52
@SiobhyB
Copy link
Contributor Author

SiobhyB commented Jun 3, 2021

To close the loop: I've created an issue for the Android Studio error here: #14773. As I'll be working on that class as part of my HACK week project, I can spend some time trying to see if it's something I can figure out next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks. [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants