[RNMobile] Prevent "Undo Level" after Setting Featured Image via Image Block #33057
Conversation
"editEntityRecord" includes an "undoIgnore: true" parameter that makes it possible to ignore an edit in the undo history. Reference: https://developer.wordpress.org/block-editor/reference-guides/data/data-core/#editEntityRecord With this commit, that parameter is set to true so that a new undo level *isn't* created after the featured image is updated via the app.
SiobhyB
added
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.07 MB ℹ️ View Unchanged
|
SiobhyB
changed the title
|
Noting here that this PR isn't a direct fix for the core issue, which is that the post's featured image doesn't update in accordance with the undo/redo actions. Instead, it's more of a workaround where a new "undo level" simply isn't created when an image is set as featured via the image block in the mobile app. While exploring possible solutions to this issue, I experimented with the following thoughts:
Although the "ideal" solution would be for the post's featured image to actually change with the undo/redo functionality, I think the workaround in this PR may be the best compromise given the above considerations and the issue's relatively low priority. @hypest, I wondered if I could get your thoughts on this whenever you have the chance? This isn't a high priority and I wanted to ask you as you're the most familiar with how data is currently flowing for this issue. I'd be really grateful to hear your thoughts and whether I'm possibly missing another approach. I'll keep this issue as a draft until I'm a bit more certain on the approach to take. |
Aha, interesting problem to have in our hands Siobhan, thanks for pinging. The way I see and understand it, I would explore how perhaps to attach a "side effect" action to undo/redo since, in the current way we have the data flow set up, we expect the native side to update the internal state of the Alternatively, but perhaps a bigger refactor could be to change the data flow in such a way that the editor would only depend and change the internal state kept in the |
|
Thanks @hypest, that makes sense and sounds inline with what I'd been experimenting with, without success so far. I'll continue experimenting, as it sounds like there should be a way to achieve this and it'd be preferable to preventing the undo level. |
Indeed, I feel that suppressing/preventing undo/redo would mainly be perceived as a bug by the user anyway so, maybe not much of a value exchanging one buggy behavior with another. That said, this has the signs of a fix that could spiral out to a whole project, so we might need to reassess our plan after the additional exploration. |
|
I've added details to the Future Considerations this PR's description to give further context around why we've decided to go this route, following the existing comments here and internal discussion. @fluiddot, I've requested your review, and welcome anyone else's opinion here. 🙇♀️ I'd especially love to hear feedback on text I've added to the confirmation notice: |
I think the message is clear but I'm concerned about the small amount of time to read it, I'm wondering if we could display it within the block settings, similar as we do in other cases like in the Columns block, wdyt?
|
|
@fluiddot, good idea! I'd only be a little worried about possibly cluttering the settings with details that may not be of importance for the majority. But, I agree it'd be more readable than adding this to the notice. For the mock of this, I updated the text to the following:
I think it works a bit better than the notice changes. Any thoughts on its design or the updated text? cc-ing @iamthomasbishop for thoughts and the final say on this, too. |
Good point, from my POV I think it makes sense to have it here but I agree that may not be of importance for the majority. However, at the same time, if we don't consider it important enough probably we shouldn't even display it in the notice 😄 .
Not sure if it's doable but I'd expect the message to be right after the link and before the separator line, otherwise it feels like it's not related to it, wdyt?
The text looks great and very clear to me 👍 . |
|
Thanks, Carlos! 🙇♀️ I'm in favour of having the text in the settings given further thought. Although it might not be relevant to everyone accessing the setting, I do think it holds some valuable details that we shouldn't completely leave users to figure out by themselves.
I kind of like the text after the separator, so that it doesn't completely detract attention away from the button, but see your point of view too and could be swayed in either direction. @iamthomasbishop, do you have any strong opinions here? If not, I'll go with Carlos' preference and update the PR for review. |
|
@SiobhyB Thanks for the ping! A few things I'd like to suggest, with hopes of finding a balance between clarity and simplicity here.
Here is a quick sketch of what I'm imagining for these changes:
Separately, I want to propose a couple of other things now that we've seen this in use for a bit. These can probably be split into other tickets and aren't urgent.
Let me know if you have any thoughts or disagree w/ any of the above. Happy to iterate on it, and it'd probably be good to get a copy review from editorial when we get to a good place 😄 . |
By adding the settings for changing a featured image under their own section heading, it's being clearly separated from the other sections. This will help with the overall alignment/rhythm of the settings panel, which is important given we're adding an additional note.
|
Thank you so much for the help, @iamthomasbishop!
I'm all for the idea of making the wording more concise, I agree the previous copy is verbose and likely overly cautious on my side. I worry that users won't recognise what we mean by "undo/redo" and wanted to be careful not to give an impression that they're not able to revert changes to their featured image at all. How about tweaking your wording a little to make it abundantly clear what we mean by "undo/redo"?
That's a good idea! I've added the
I'll address your other feedback in separate comments too. |
SiobhyB
added
the
Needs Copy Review
|
Thank you so much @clucasrowlands. ❤️ I've gone ahead to update the text to be
@iamthomasbishop, I went ahead to remove the bottom border from the button in 89f15ac and also removed the bottom padding in 6ac5e79, with the following results:
Removing the bottom padding actually served to increase the spacing between the top of the button and the section's "Featured Image" header. I believe that's related to flexbox. I was going to dig deeper but then realised the spacing between the section's header and the button now matches the amount of spacing between other section headers and their content e.g. the "Link Settings" header and the "Link To" tab. I wanted to check with you to see if this is a desired end point or if you'd prefer to see some more tweaks around the spacing here? Thanks in advance! |
@SiobhyB Just wanted to clarify, I'd like to keep the button sizing as-is so that we provide an adequate tap-target size. So the heading and footer cells would get collapsed, like this:
Looking closer at your "after" screenshot above, it looks like the spacing between header/button/footer is equal, but in practice it looks like a bit too much space. Would it be possible to remove the spacing here?
I'm assuming that will require a change to the cell components themselves, so if we're not able to do this easily, I'm okay with either of the options in your before/after above — with a slight preference toward the latter. |
|
@iamthomasbishop, I've been having a bit of a difficult time trying to style the components in the way set out in the sketch today. The padding beneath the I also believe the current spacing is a result of flexbox and have been trying to change things, but again no luck in getting close to the sketch. @fluiddot, do you perhaps see any approaches or options I'm missing here? |
|
@SiobhyB That all makes sense. If it makes it easier, let's just roll with your "before" example above—meaning the one that has the separator between the button and footer cell, like this:
I was on the fence, but I think having a separator between is probably "technically" correct from a design perspective so we can roll with that. |
With this commit, the content in the "Set as Featured" button and the footer message is aligned to the top of their containers. This will prevent flexbox from automatically adding spacing to the top of their containers, which goes against the design we want.
As we need to tweak the spacing around the panel's title, I've added a new "titleStyle" prop to the PanelBody component. That prop is then used to remove the bottom spacing for the "Featured Image" panel's title. The prop can now be re-used by anyone else who needs to tweak the title in a PanelBody in the future.
Following the previous changes, this commit increases the height and "tappable area" of the "Set/Remove as Featured Image" button by adding both top and bottom padding.
|
@iamthomasbishop, thanks for your continued help/feedback here! I've been able to get this closer to the design you set out here following a session with @fluiddot:
Let me know if you'd prefer a change in the padding or if you want the border at the top of the footer note, as that should be fairly straightforward to change now. @fluiddot, I think the PR is ready for your review now, with the note that there may be some small changes to the CSS following any further design feedback. :)
|
|
That looks great @SiobhyB , thank you! |
Due to an error when merging, the code was referencing an old function name, "getSetFeaturedButton". It should instead reference the name name, "getFeaturedButtonPanel".
For consistency with other selectors, this commit updates the name of "setFeaturedCellContainer" to "setFeaturedButtonCellContainer".
| <PanelBody> | ||
| <FooterMessageControl | ||
| label={ __( | ||
| 'Changes to featured image will not be affected by the undo/redo buttons.' | ||
| ) } | ||
| cellContainerStyle={ | ||
| styles.setFeaturedButtonCellContainer | ||
| } | ||
| /> | ||
| </PanelBody> |
There was a problem hiding this comment.
I was wondering if we really need to have the footer message under a new panel body, if the purpose is to prevent rendering a separator, you could disable it by passing the prop separatorType="none" to the BottomSheet.Cell components defined in the getFeaturedButtonPanel function. This way you could render the FooterMessageControl component in the same panel body as the featured button.
There was a problem hiding this comment.
You're right that there's no meaningful reason for the footer message to exist in its own panel body. I've gone ahead to move it within the featured panel in 5662508, and have removed the separator as per your suggestion. :)
The placement of the component is moved within the main "Featured Image" panel, as this is more semantically correct than having the component within its own, separate panel.
There was a problem hiding this comment.
LGTM 🎊 ! Amazing work @SiobhyB 💯 !
I tested this PR on Samsung Galaxy S20 FE 5G (Android 10) and Simulator iPhone 11 Pro Max (iOS 14.4).
NOTE: When opening the Image block settings, I noticed that the following error was logged in the console:
ExceptionsManager.js:180 Warning: Cannot update a component (`Unknown`) while rendering a different component (`BottomSheetTextControl`). To locate the bad setState() call inside `BottomSheetTextControl`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render
in BottomSheetTextControl (at edit.native.js:416)
Looks like it's related to this line because if I set an empty function on that prop the error stops. I think it's out of the scope of this PR but I wanted to let you know as it might have been introduced in this PR, as per this commit.
|
Thank you so much for your help, @fluiddot! I'll address the error you've noted in a separate PR. 🙇♀️ I'll go ahead to merge this PR and also wanted to sum up the following pending issues that have been discussed in the comments:
The plan is for me to start diving into different areas of the codebase, so there is no guarantee of when the above items will be prioritized or if I'll be the one to work on them in the future, but they're at least documented and I'd be happy to help in any way I can with any of them. Thanks to all involved on this project so far for the help and patience. :) |
This commit aims to fix the render error outlined here: #33057 (review) The "outside" changes that happen when text is changed in the BottomSheetTextControl component should not be called directly from that component. Instead, they're now called within the useEffect hook.
) * Resolve render error This commit aims to fix the render error outlined here: #33057 (review) The "outside" changes that happen when text is changed in the BottomSheetTextControl component should not be called directly from that component. Instead, they're now called within the useEffect hook. * Replace "value" prop with "defaultValue" The purpose of this commit is to address the text entry issues that currently exist for the TextInput component, as described here: facebook/react-native#30503 A working workaround propose in that thread is to replace the "value" prop with "defaultValue". * Simplify component by removing unecessary hooks The introduction of the "defaultValue" prop in f6d9697 means that it's no longer necessary to keep the value in state. "defaultValue" allows for text to be automatically changed as it's typed. See: https://reactnative.dev/docs/textinput#defaultvalue We don't need to use "onChangeText" for the purpose of updating the input's text and can instead use it to invoke the "onChange" function. We can also remove the useState and useEffect hooks, as they're not necessary to update the component's value.
This PR is intended as a temporary workaround for #32937
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#3682Description
As outlined in #32937, the undo/redo functionality doesn't work as expected after setting a featured image via the image block.
After setting a featured image via the block and then tapping undo, it appears as though the action has successfully reverted, as the Featured banner is removed from the block. However, the image still remains as the post's featured image.
With this PR, the issue is worked around by preventing an "undo level" from being created after a featured image is set via the image block. Tapping undo or redo immediately after setting a featured image via the image block should no longer have any effect.
Note, this PR doesn't solve the original issue. In fact, it will introduce a new UX issue, since the undo/redo buttons won’t work as the user would expect i.e. an undo will revert changes they made just before setting a featured image, rather than the featured setting, and may be confusing. However, this inconvenience should be considered preferable to the current experience in production, where changes to the featured image via the image block are not saved, despite the UI indicating the undo is successful.
The Future Considerations section of this PR can be referred to for further hopes/considerations for any future iterations.
How has this been tested?
Screenshots
The following screenshots show the before and after footer in the image settings, with the updated footer noting that the undo/redo buttons won't work as expected for users:
Types of changes
This PR introduces a bug fix (a non-breaking change that fixes an issue), with the following main changes to the code:
editEntityRecordis currently triggered whenever a request to change the featured image is received from the apps (i.e. when theSet/Remove as Featuredbutton is tapped from the image block). That action creator includes anundoIgnoreparameter that makes it possible to ignore an edit in the undo history. For our purposes, that parameter is set to true so that a new undo level isn't created after the featured image is updated via the app.Changes to featured image will not be affected by the undo/redo buttonsFuture Considerations
As a more permanent fix for the original problem, the team discussed adding side effects to the undo/redo buttons, however, this deviates a lot from existing patterns in Gutenberg and is something we decided against pursuing.
The ideal fix would involve completely refactoring the existing data flow so that Gutenberg’s store is the primary location that the apps read from. A refactor would involve the following non-trivial changes:
Checklist:
*.native.jsfiles for terms that need renaming or removal).