[RNMobile] Add "Set as Featured Image" Button to Image Block (iOS Only) #31415
Conversation
…ImageId() over the bridge gutenbergFeaturedImageId() will be used to gather the initial ID of a post's featured image when the editor first loads in iOS.
…featured image over the bridge
"border-radius" doesn't function as intended on iOS devices without use of "overflow: hidden".
…er.swift This is to align the code in the demo file with the code in GutenbergBridgeDataSource, so that it runs as expected.
…/featured-badge-ios
SiobhyB
added
the
Mobile App - i.e. Android or iOS
SiobhyB
changed the base branch from
trunk
to
add/featured-functionality-to-image-block-ios
|
Size Change: 0 B Total Size: 1.04 MB ℹ️ View Unchanged
|
…bergViewController This update ensures the function conforms to the pattern set out for it in GutenbergBridgeDelegate
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
cf57b05
to
53f3ec5
Compare
…/set-as-featured-button-ios
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
f50b46e
to
5cbeb7b
Compare
…/set-as-featured-button-ios
…/set-as-featured-button-ios
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
219d68b
to
0bcc7ca
Compare
…/set-as-featured-button-ios
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
b804297
to
bd7bfa0
Compare
…/set-as-featured-button-ios
|
Testing wordpress-mobile/WordPress-iOS@1a8758a on iPhone 11 Pro Max simulator. Test Case 1: General Flow
Test Case 2: "Replace Existing" Flow
Test Case 3: "Editor Loads" FlowNo new feedback. Test Case 4: "Post Settings" FlowNo new feedback. Test Case 5: "Replace in Image Block" Flow
@SiobhyB, great work here and I appreciate the easy to ready testing instructions 👍. I haven't had a chance to look at the code but will do that next. |
Following feedback, the getSetFeaturedButton function has been re-named to getFeaturedButtonPanel for clarity.
|
Thanks so much for your thorough review, Paul! I'll go ahead to work through it all and address feedback from each "test case" in a separate comment. |
Response to Feedback from Test Case 1
Ah, thank you for sharing that post! I enjoyed the writer's perspective and it reminded me of similar encounters with users, who weren't aware of certain features due to a lack of scroll indicators. As the issue can be found on all BottomSheets with overflowing content, it seems like this may be larger than this specific PR. I've gone ahead to create an enhancement request at #32409. Let me know if you think any more action is required around this specific PR.
I wasn't able to reproduce an issue specific to this branch. Did you test using the same image, with the same size set? I do see that the edit/replace button appears outside of the image in cases where the size is set to anything other than
I'm able to replicate the flickering but having a difficult time tracking down why it's happening. I'm only able to replicate it when a featured image is set/removed straight away (i.e. the replace dialog isn't used), so I wonder if it's something related to the BottomSheet dismissal. 🤔 I'll keep exploring. |
Response to Feedback from Test Case 2
This was fixed in wordpress-mobile/WordPress-iOS@02db104, thank you so much for spotting that!
I appreciate the extra context around how other titles work in the app. I like
I agree that there may be some confusion around which image is set as featured. Thomas mentioned this in a design review, also, and I've opened an enhancement request at #30410. Although I'll be focusing on other areas of code after this project, I've added it to a list that I hope to get to someday, perhaps a future HACK week when I've built up my knowledge a bit more. :)
I initially viewed these as headings of sorts, which is why I went with title case, but I think that's a bit of a debatable classification and you're right that title case isn't really appropriate, especially as other notices don't use title case in the app. I've gone ahead to update this for iOS in wordpress-mobile/WordPress-iOS@7e7a291 and for Android in wordpress-mobile/WordPress-Android@c1f6842. |
Response to Feedback from Test Case 5This is really useful feedback that matches some other internal chatter, thank you! I'll keep monitoring feedback like this and consider tracking it via a new GitHub issue if it continues to be a theme. 🙇♀️ |
…/set-as-featured-button-ios
Thanks for making that issue! I agree it isn't specific to this PR so happy to see it left out for now.
I'm having trouble with the iOS build at the moment, but once that's done I'll come back and test this to make sure I see the same thing on this branch and on
I've seen some minor quirks with that notice. For example, if you delete a couple of blocks one-after-the-other, the "Block removed" notices get queued up which is probably unexpected (at least in that scenario). I haven't seen this particular flickering issue anywhere else though.
No worries!
Sounds good!
Looks great, thanks for creating that issue.
🙇
Glad to know I'm not alone raising that feedback. |
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
761af27
to
ad77029
Compare
…/set-as-featured-button-ios
With this PR, the featured button will work on iOS and this const will no longer be needed.
Thanks for this extra context! I did some testing and am also able to reproduce some quirkiness with the notice on Android. If I'm not able to find a straightforward fix as part of this PR, I might lean towards creating a separate issue. But I'll spend a bit more time experimenting tomorrow first. 🙇♀️ |
I think your comment earlier was spot on, it has to do with the Image size setting and has nothing to do with this PR. |
|
I gave this another test today and spotted one issue: Sometimes the Featured Image section in Post Settings displays an "Upload Failed. Tap for options." message when I select and then deselect featured images. Then when I retry from the Post Settings screen, it loads an image from a previous post (not the image from the post I'm currently on): retry-diff-image.mov |
An image can't be set as featured unless it's been successfully uploaded to the site's media library, but the 'canImageBeFeatured' check didn't account for this. A temporary negative ID is given to images that are in progress or fail to upload. With this commit, a check for whether an image's ID is more than zero is added to 'canImageBeFeatured' in order to account for those times. Note, 'this.state.isUploadInProgress' isn't used, as it doesn't account for times where an upload fails. Instead, a ID check acts as a catch all.
|
@guarani, thank you for spotting that! I think what might be happening is this:
I've added a check to ensure the button only displays when an image's ID is greater than zero in 60d205c as a way to avoid this. That said, I have some uncertainty as it seems like the image in your video is fully uploaded and I had a bit of trouble reliably replicating. I was able to replicate, but only following a lot of trial and error with forcing an upload to fail while in progress (by turning the WiFi off). I'm not able to replicate the issue any more but, if you're still able to replicate on your side even with the latest change, would you be able to add the following logging within the I'd be interested to know what the output is after you first click the |
…/set-as-featured-button-ios
SiobhyB
force-pushed
the
add/set-as-featured-button-ios
branch
from
92b9f7a
to
4437cac
Compare
…/set-as-featured-button-ios
There was a problem hiding this comment.
Testing wordpress-mobile/WordPress-iOS@8f47364 on iPad 8th gen:
- Test Case 1: General Flow
- Test Case 2: "Replace Existing" Flow
On iPad, cancel buttons are not rendered (which is expected I think) so tapping outside the action sheet works instead of cancelling. - Test Case 3: "Editor Loads" Flow
- Test Case 4: "Post Settings" Flow
With regard to setting a featured image via Post Settings and then adding the image to the editor via the image block, the featured image banner only shows up if I add the image from the WP Media Library, not the device. I think this is expected though, since adding an image from the device is treated as a different image to the one that's in the WP Media Library, even if they're the same image. - Test Case 5: "Replace in Image Block" Flow
Same applies here where replacing an image only restores the featured image banner if the image is re-added via the WP Media Library.
I don't think any of the above are issues, just wanted to note them here.
Looks great and works great, thanks @SiobhyB!
…ctly from Image Block (iOS Only) (#31345) * Update CHANGELOG.md with feature details * [RNMobile] Add Featured Banner to Image Block (iOS Only) (#31347) Add a "featured" banner that overlays the image block on iOS devices. * [RNMobile] Add "Set as Featured Image" Button to Image Block (iOS Only) (#31415) Add "Set as Featured Image" Button to Image Block * Update CHANGELOG.md
Partial fix for: wordpress-mobile/gutenberg-mobile#1011
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3450WordPress-iOS: wordpress-mobile/WordPress-iOS#16426The main PR for the branch this individual PR will be merged into,
add/featured-functionality-to-image-block-ios, can be found here: #31345Description
Building on the work done in #31347, this PR will add 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.
How has this been tested?
Test Case 1: General Flow
To start with, the general flow for setting a featured image via the image block on a post without an existing image is as follows.
Test Case 2: "Replace Existing" Flow
It's also possible for users to replace an existing featured image via the following flow.
Test Case 3: "Editor Loads" Flow
Next, we should verify that a **Remove as Featured** option appears when a post with a featured image is first loaded. To test this, follow these steps.
Test Case 4: "Post Settings" Flow
The image block should be updated to reflect whether its image is featured or not even when the featured image is set via the general Post Settings flow.
Test Case 5: "Replace in Image Block" Flow
Finally, it's possible to replace an image directly within the image block's settings. We need to verify that the featured settings update as expected depending on the image that's chosen for an image block.
Screenshots
Types of changes
This PR introduces a new feature (non-breaking change which adds functionality). The feature it introduces is a Set as Featured button in the image block's setting, which enables a user to either set an image as featured or remove it as featured. A high-level overview of the code changes involved is as follows:
getSetFeaturedButtoncomponent deployed in the Android version of this PR ([RNMobile] Add "Set as Featured Image" Button to Image Block (Android Only) #28854) to display aSet as Featuredor aRemove as Featuredbutton, depending on whetherisFeaturedImageis true or not. Further details around the logic behindisFeaturedImagecan also be found in that PR.setFeaturedImagewill be called when the button is tapped, this function utilizes another function namedgutenbergDidRequestToSetFeaturedImageto send an image's media ID over the bridge when a request is made to either set a new featured image (in which case the media ID is that of the selected image) or to remove an existing featured image (in which case the media ID is zero).featuredImageIdNativeUpdatedlistener, keeping the native apps as our main source of truth when it comes to confirming the change to the post's featured image ID.Checklist:
*.native.jsfiles for terms that need renaming or removal).