[RNMobile] Adds Clipboard Link Suggestion to Image block and Buttons block #35972
Conversation
```
console.error
Warning: Each child in a list should have a unique "key" prop.
Check the render method of `StylePreview`. See https://reactjs.org/link/warning-keys for more information.
...
86 | >
87 | <View style={ [ styles.container, { width: itemWidth } ] }>
> 88 | <View style={ styles.imageWrapper }>
| ^
89 | { isActive &&
90 | getOutline( [ styles.outline, innerOutlineStyle ] ) }
91 | <Image
```
…the whole test runtime.
This will help avoid inadvertently making async I/O operations to underlying Native Modules
or HTTP when integration-testing our components to ensure more stability in the test runtime.
+ Mocked `Image.getSize` specifically, i.e., not the whole `Image` module from `react-native`.
+ Rewrote the implementation to simply call the provided success callback synchronously.
+ It returns default values of `0` for `width` and `height` pretending to have retrieved the image URI async using HTTP.
+ Mocked `Clipboard.[gs]etString` which is the entire `Clipboard` module from `react-native`.
+ **Note:** `Clipboard` was deprecated in favor of `@react-native-community/clipboard` and will be removed in the future.
+ Rewrote the implementation to simply do nothing on `setString`.
+ Rewrote the implementation to simply return a `Promise` that resolves to `''` on `getString`.
+ Staging strings returned from the `Clipboard` in future integration tests can be as simple as `Clipboard.getString.mockReturnValueOnce( 'value' )`.
…ipboard into the `Link Settings > Link To` field when editing an `Image` block type. + Deliberately removed existing unit tests in this module for the moment in order to make transitioning from `react-test-renderer` to `@testing-library/react-native` easier to manage. They will either be re-added in the near future, or converted to complementary integration tests. + Deliberately structured the test cases in "Given-When-Then (GWT) Gherkin-style." + Added the first test that necessitates the change for forcing auto-population of URLs from the Clipboard to NOT take place when editing an `<ImageEdit/>` block. + Intend to parameterize the tests to see how the same tests may be leveraged for other block types that have `Link Settings`, e.g., `Embed` and `Button` to avoid repeating the tests for those block types. + Integration tests will eventually be written for various planned user experience scenarios including: ``` GIVEN the Link Settings sheet displays, AND the Clipboard has a URL copied, THEN the `Link to` field in the Link Settings should be populated with the placeholder text, i.e., `Search or type URL`. GIVEN the Link Picker sheet displays, AND the Clipboard has a URL copied that is different from the contents of the text input field, THEN the `From Clipboard` table cell should be populated with the URL from the Clipboard. GIVEN the Link Picker sheet displays with the `From Clipboard` table cell, WHEN the Clipboard is cleared or changed to something that is NOT a URL, THEN the `From Clipboard` table cell should disappear. GIVEN the Link Picker sheet displays, AND the contents of the Clipboard are IDENTICAL to the contents of the text input field, THEN do NOT display the `From Clipboard` table cell. GIVEN the Link Picker sheet displays, AND the `From Clipboard` table cell is pressed, THEN the `Search or type URL` text input field is populated with the URL from the Clipboard, AND the `Add this link` table cell is repopulated with the new URL from the Clipboard. ```
ttahmouch
changed the title
…dit/>` block to the `<ButtonsEdit/>` block. + Added a utility function called `interactionWithUIElement` to interact with an element under test before assertions. This allows us to pretend that the user is interacting with the UI of the `<Editor/>` as if they are using the mobile app in order to stage a test. Right now it's fairly primitive, and is mostly used for `press` events. It can also be used for `layout` events at the moment, but it would need to be elaborated further to support events like `changeText` or `scroll`. + Used `interactionWithUIElement` to generalize a test to assert the same expectations regardless of whether the UX is displaying an `<ImageEdit/>` block or `<ButtonsEdit/>` block. The expected UX the test is asserting should be identical regardless of which block is being used in the `<Editor/>`. The only technical difference is how we'd find the elements in the view tree since they have different accessibility props to identify them, e.g., `getByA11yLabel` for the edit button block, and `getByA11yHint` for the edit image block.
|
Hey @ttahmouch 👋 |
… Link Picker **SHOULD/NOT** display to the user. + Tested some naive scenarios where the `From Clipboard` button **WOULD** or **WOULD NOT** display given the `Clipboard` has a **VALID** or **INVALID** URL copied. It is naive because it doesn't account for if the user has already copied the URL suggestion from the `Clipboard` into their Link Settings yet. Presumably, we **SHOULD NOT** suggest the URL if it is already the URL in their Link Settings (which will be a future test assertion). + Added a [Clipboard SVG](https://www.iconfinder.com/icons/211649/clipboard_icon) as a placeholder asset for the `From Clipboard` button until a final one is decided. It is MIT licensed. + Reimplemented `interactionWithUIElement` to **NOT** be an `async` function to avoid unnecessary warnings, i.e., `Warning: An update to ForwardRef(NavigationContainer) inside a test was not wrapped in act(...).`.
…om the `<ImageEdit/>` Block Library module as they are not specific to the Image Edit Block but also the Button Edit Block.
…s hidden or pressed in the Link Picker. 0. Hiding the Clipboard Link Suggestion. Previously, the Clipboard Link Suggestion was only being hidden in the Link Picker if the text in the Clipboard was NOT a valid URL. Now, the Clipboard Link Suggestion will also be hidden in the Link Picker if the text is a valid URL, but is the same URL the user already picked. 1. Pressing the Clipboard Link Suggestion. Previously, this was unimplemented. Now, the behavior is identical to selecting a Link Suggestion from the search results in the Link Picker, i.e., it will navigate back to the previous screen and pass it the selected URL to change the Block Attributes.
ttahmouch
changed the title
…uggestionItemCell/>` to be localized. The tests still default to the `English` translation.
TODO
|
There was a problem hiding this comment.
Great work @ttahmouch 👍
I tested the functionality using metro on Android and everything works as expected for me! The code also LGTM on a first pass and I have no comments at this point 🎉
The added tests also look good. Thank you for taking the effort to add those 🙇
I have a suggestion regarding the behavior when switch back to the app with new contents in the clipboard. In that case the suggestion is not updated. I don't consider this a major issue and I think that we should handle this in a separate PR (if catching this case is feasible).
device-2021-11-18-144847.mp4
Since the added functionality is sufficient I would suggest to wrap-up this PR as is without extending the functionality/scope at this point (since the PR is already 500 lines) and marking it ready for review. We can then follow up by opening separate issues/PRs for any functionality or code improvements. Wdyt?
antonis
added
[Block] Buttons
antonis
added
Mobile App - i.e. Android or iOS
antonis
changed the title
packages/components/src/mobile/bottom-sheet/link-suggestion-item-cell.native.js
Show resolved
Hide resolved
|
Apologies for the delay, @antonis . I started replying earlier, and then got distracted.
Thank you for taking the time to thoroughly review the branch. I appreciate you smoke testing the code on
That functionality crossed my mind during development. I just never ended up circling back around to it.
The PR is rather large now, yes. Granted, However, I agree. If we were to add this additional enhancement, there's no reason that should block this PR from moving forward. Being able to change the contents of the Does that make sense? |
packages/block-editor/src/components/block-styles/preview.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/bottom-sheet/link-suggestion-item-cell.native.js
Show resolved
Hide resolved
|
@ttahmouch Thanks for cranking this out, looks great! I just noticed one tiny thing. I thought it might just be a weird artifact in one of the videos above, but I'm seeing the same thing in all of them. There is a very small (~6-8px) diff in spacing, which results in a "jump" when the cell switches between "from clipboard" and a regular cell.
My guess is that there is supposed to be a bit more padding at the top or above the cell, which would bring the new "from clipboard" cell (on the left in the example above) down to align with the other cell (on the left right in the example above), but I'm not 100% sure. |
I think this is a related issue to the Clipboard Link Suggestion not being in the same list as the Search Result Link Suggestions that I noted in this PR. It's been on my TODO list to move the Clipboard Link Suggestion to technically be in the same list, and hopefully that should correct all margin-related issues. Thanks for letting me know, @iamthomasbishop . |
| * | ||
| * @see node_modules/react-native/Libraries/Components/Clipboard/Clipboard.js | ||
| */ | ||
| jest.mock( 'react-native/Libraries/Components/Clipboard/Clipboard', () => ( { |
There was a problem hiding this comment.
@ttahmouch Looks like the Clipboard library is also mocked line 189, I recall I also had to add it for other integration tests. I don't think this would cause any conflict but might be confussing having duplicated code, wdyt?
There was a problem hiding this comment.
Thanks for letting me know, @fluiddot . I think I remember you adding it for work related to the Embed Block that you and Joel implemented.
I think this may have slipped by when I merged the trunk branch downstream into my feature branch to keep it up-to-date before release. I can remove the redundancy.
There was a problem hiding this comment.
No worries, it happened to me several times 😅 . I wish Git could detect these redundancy cases although they are not trivial at all to infer.
| </div> | ||
| <!-- /wp:button --> | ||
| `, | ||
| toJSON: () => 'core/button', |
There was a problem hiding this comment.
@ttahmouch I'm wondering if we need this property as I don't see being exposed and used in the function at line 58, should we remove it?
There was a problem hiding this comment.
It is actually used at line 58 just not in the way you'd think.
If the value has a toJSON() method, it's responsible to define what data will be serialized.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify
'<LinkSettings/> from %j' is serialized with the test arguments toJSON to make it clear in the console logs that this test suite iteration is for a different block under test, i.e., core/button then core/image.
I can remove it, but I felt it to be useful. WDYT?
| // Act | ||
| try { | ||
| const block = await waitFor( () => | ||
| subject.getByA11yLabel( | ||
| type === 'core/image' ? /Image Block/ : /Button Block/ | ||
| ) | ||
| ); | ||
| fireEvent.press( block ); | ||
| fireEvent.press( block ); | ||
| fireEvent.press( | ||
| await waitFor( () => subject.getByA11yLabel( 'Open Settings' ) ) | ||
| ); | ||
| } catch ( error ) { | ||
| done.fail( error ); | ||
| } | ||
|
|
||
| // Assert | ||
| try { | ||
| await waitFor( () => | ||
| subject.getByA11yLabel( | ||
| `Link to, ${ | ||
| type === 'core/image' ? 'None' : 'Search or type URL' | ||
| }` | ||
| ) | ||
| ); | ||
| done(); | ||
| } catch ( error ) { | ||
| done.fail( expectation ); | ||
| } |
There was a problem hiding this comment.
@ttahmouch I'm out of curiosity about the use of try-catch in these tests, in case we could replicate it on future tests. I understand that the purpose is to control the message displayed when the test fails, right?
There was a problem hiding this comment.
That's correct. I didn't feel that the default error message is particularly expressive or helpful for the final test expectation. Though I considered just decorating the original {Error} instance emitted by react-native-testing-library. I can still consider changing it to log the original error as well if you think it would be helpful.
WDYT?
There was a problem hiding this comment.
No, I think it's great providing more accurate error messages, although it shouldn't be the common case as tests should succeed 😅 . Checking the Action section, I'm wondering if it's necessary the first try-catch there because if any error is raised the test would fail in any case before entering the Assert section, right?.
| getURLFromClipboard() | ||
| .then( ( url ) => setValue( { value, clipboardUrl: url } ) ) | ||
| .catch( () => setValue( { value, clipboardUrl: '' } ) ); |
There was a problem hiding this comment.
In order to avoid React errors regarding setting state on an unmounted component, we may want to avoid updating the state with a clean up function for the Hook. WDYT?
useEffect(() => {
let isCurrent = true;
getURLFromClipboard()
.then((url) => {
if (isCurrent) {
setValue({ value, clipboardUrl: url });
}
})
.catch(() => {
if (isCurrent) {
setValue({ value, clipboardUrl: "" });
}
});
return () => {
isCurrent = false;
};
}, []);
Description
How has this been tested?
I smoke tested the change in an
iOSsimulator to see its behavior at runtime. I have not checkedAndroidyet.I also wrote automated integration tests for the UX behavior.
Designing the Expectations
We had a brief discussion early on before writing any code to understand how the UX should behave. We attempted to capture those expectations in writing to have requirements to implement against.
Designing the Tests
I integration tested the
<ImageEdit/>and<ButtonEdit/>components implicitly by testing the entire<Editor/>component from the surface. I wanted to try and make the testing more "pure" by making it resemble a functional test as closely as possible. By that I mean, I wanted to pretend that I have a screen open, and interact with the screen; not an isolated UI element, i.e.,<ImageEdit/>or<ButtonEdit/>.<LinkSettings/>component as a whole which includes the subsequent screen with the<LinkPicker/>.Link Settings, e.g.,EmbedandButtonto avoid repeating the tests for those block types.Deliberately removed existing unit tests in this module for the moment in order to make transitioning fromreact-test-rendererto@testing-library/react-nativeeasier to manage. They will either be re-added in the near future, or converted to complementary integration tests.link-settingscomponent, i.e.,packages/components/src/mobile/link-settings/test/edit.native.js. (The module should probably be renamed since it's no longer testing the "Edit Button/Image Block" module specifically.)Aside
Mocking
Image.getSizeandClipboard.[gs]etStringfor the whole test runtime.This will help avoid inadvertently making async I/O operations to underlying Native Modules or HTTP when integration-testing our components to ensure more stability in the test runtime.
Image.getSize
Image.getSizespecifically, i.e., not the wholeImagemodule fromreact-native.0forwidthandheightpretending to have retrieved the image URI async using HTTP.Clipboard.[gs]etString
Clipboard.[gs]etStringwhich is the entireClipboardmodule fromreact-native.Clipboardwas deprecated in favor of@react-native-community/clipboardand will be removed in the future.setString.Promisethat resolves to''ongetString.Clipboardin future integration tests can be as simple asClipboard.getString.mockReturnValueOnce( 'value' ).Resolving runtime error when running
npm run native test.Related Issue
Related PR
Screenshots
Before (iOS)
0-before-auto-clipboard.mp4
After (iOS)
https://user-images.githubusercontent.com/3058263/139108312-56d9b2f4-551d-4c5a-a275-da89160a9aa6.mp4Button Edit Block (iOS)
after-button-edit-480.mp4
Image Edit Block (iOS)
after-image-edit-480.mp4
GridIcons 📋 SVG (iOS)
After - Button Edit Block (Android)
android-button-clipboard.mp4
After - Image Edit Block (Android)
android-image-clipboard.mp4
Types of changes
Feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).