[RNMobile][Embed block] Fix URL update when edited after setting a bad URL of a provider #35013
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
[Type] Bug
fluiddot
commented
Sep 21, 2021
Comment on lines
+196
to
+198
| useEffect( () => setShowEmbedBottomSheet( isEditingURL ), [ | ||
| isEditingURL, | ||
| ] ); |
There was a problem hiding this comment.
This side-effect will assure that the embed bottom sheet is shown/hidden when isEditingURL is updated.
fluiddot
commented
Sep 21, 2021
Comment on lines
288
to
298
| setIsEditingURL( false ); | ||
| setAttributes( { url: value } ); | ||
| setIsEditingURL( false ); |
There was a problem hiding this comment.
I found out that the order here is important, we need to update the URL attribute before the following side-effect is triggered when the isEditingURL value is changed:
gutenberg/packages/block-library/src/embed/edit.native.js
Lines 171 to 194 in 6dd7438
There was a problem hiding this comment.
Do you think maybe we should add a comment here as well?
There was a problem hiding this comment.
Good point, I'll add a comment here to clarify it 👍 .
There was a problem hiding this comment.
I added your suggestion in this commit.
fluiddot
commented
Sep 21, 2021
Comment on lines
293
to
295
| onClose={ () => { | ||
| setShowEmbedBottomSheet( false ); | ||
| } } |
There was a problem hiding this comment.
The onClose prop should only be used for controlling the bottom sheet's visibility.
|
Size Change: 0 B Total Size: 1.06 MB ℹ️ View Unchanged
|
2 tasks
ceyhun
approved these changes
Sep 23, 2021
# Conflicts: # packages/block-library/src/embed/edit.native.js
|
Looks like the |
This reverts commit cfc8f74.
fluiddot
added a commit
that referenced
this pull request
Nov 3, 2021
24 tasks
fluiddot
added a commit
that referenced
this pull request
Nov 4, 2021
* Add testID to embed bottom sheet * Export registerBlock from block library * [WIP] Add embed block integration tests * Fix set valid URL embed integration test * Add waitForElement to initialize editor helper * Add embed preview mocked styles * Add most used embed providers insertion tests * Add test cases for setting URL by tapping on block * Add mock embed responses helper * Add edit URL test cases * Add invalid URL test case * Refactor embed block integration tests * Mock RN clipboard library * Add auto-paste URL from clipboard test cases * Use snapshots and simplify integration tests * Add update test snapshots command * Add change alignment test case * Add retry test case * Add preview coming soon test cases * Mocked RCTAztecView to utilize an underlying TextInput. * Add testID to Android version of picker * Omit style prop in Aztec mock * Add paste URL to create embed test cases * Update snapshots due to mocking Aztec * Revert "Update snapshots due to mocking Aztec" This reverts commit 2114925. * Unmock react-native-aztec for some block tests * Remove commented code * Embed block integration tests part 2 (#35533) * added test for Embed block caption. * WIP * fixed unneeded diff change. * WIP block settings * Mocked RCTAztecView to utilize an underlying TextInput. * Fixed Embed block caption test issues. * Created test - toggle resize for smaller devices media settings * Added cannot embed test. * Removed unneeded test id. * WIP insert embed from slash inserter. * Mock fetch request in cannot embed test case * Trigger onSelectionChange event instead of onChange * Query slash inserter item by text * Add expected HTML to slash inserter test case * Mock autocomplete component styles * Set paragraph as default block * Add empty paragraph HTML constant * Add test suite for insert via slash inserter case * Update toggle responsive test case * Fix request mock for theme endpoint * Add slash inserter cases for most used providers * Expect for block settings button instead edit URL button * Use snapshot testing instead of checking HTML * Add block settings test suite * Add embed test snapshots * Use snapshot in insert generic embed block test Co-authored-by: Carlos Garcia <[email protected]> * Use promise in initializeEditor to prevent act warnings * Simplify tests using initializeWithEmbedBlock * Add test case to cover an already fixed bug * Add test case to cover an already fixed bug (#35013) Co-authored-by: Joel Dean <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
gutenberg-mobilePR: wordpress-mobile/gutenberg-mobile#4002Description
One of the uses of the
isEditingURLvalue is to control the visibility of the embed bottom sheet, as it matches the expected behavior for this. However, I noticed that this value is also used to trigger a side-effect that replaces the block, producing that a new block is added before the URL was actually edited (more info can be found in this issue).This PR introduces a new value in the local state of the embed block to specifically control the visibility of the bottom sheet and prevent undesired side effects.
How has this been tested?
Screenshots
N/A
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).