Rich text: keep block ID on split #28505
Merged
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
ellatrix
requested review from
ajitbohra,
cameronvoell and
guarani
as code owners
Closed
6 tasks
|
Size Change: +69 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
ZebulanStanphill
added
[Block] Heading
|
I think this should also fix #26252. |
dratwas
reviewed
Jan 27, 2021
There was a problem hiding this comment.
Thanks for this PR @ellatrix. Seems like it is almost the same as my PR :)
I'm still wondering what you meant in this comment by Adding keepId is a horrible API. https://github.com/WordPress/gutenberg/pull/28101/files#r564409051
What is the difference between your approach and mine? Is it only about the naming? (except the mutation of a newly created block)
| @@ -296,6 +296,10 @@ function RichTextWrapper( | |||
| const hasPastedBlocks = pastedBlocks.length > 0; | |||
| let lastPastedBlockIndex = -1; | |||
|
|
|||
| // Consider the after value to be the original it is not empty and | |||
| // the before value *is* empty. | |||
| const isAfterOriginal = isEmpty( before ) && ! isEmpty( after ); | |||
There was a problem hiding this comment.
Hmmmm I'm not sure if I'm right but that means if we create a new block by pressing "return" we actually add a new block before the original one which is kinda confusing.
On the other hand, I'm wondering if it solves the issue from here
especially this point:
- If there was anything attached to that entities' id, like metadata, that won't be there anymore. Or if anything references those ids, that tracking will be lost.
I think that the clientID should stay the same only for the true original block (the one that is selected when we hit the "return" key)
There was a problem hiding this comment.
I think that makes sense. Nothing changes about the second block in that case and you insert a block before the selected block essentially. Think about a heading. If you press enter before the heading, you expect the heading to stay intact and a paragraph before the heading.
There was a problem hiding this comment.
Just checked how it works in google docs and if you press enter at the beginning of the heading, the heading style is still there but I'm not a UX guy tho. However, I would love to hear more opinions about that.
|
I'm not a big fan of this approach and as mentioned it's not intuitive for me, however, if it makes sense for you I will not argue with that.
I haven't fixed this intentionally. I asked on the slack channel if it should work like that and was waiting for the response. I also prefer to make one fixe per PR, especially in a project of a scale like this;) Ok, closing my PR and waiting for the merge of this one since we still will need to add |
gwwar
approved these changes
Feb 11, 2021
There was a problem hiding this comment.
This tests well for me @ellatrix! I think folks depending on stable block ids is a little unusual, and worth enforcing in unit tests (in a possible follow up) to avoid breakages and help document expected behavior. Some example cases might look like: press enter in middle of paragraph, start of paragraph, end of paragraph, etc.
Let's make sure the admin tests are green before merging, looks like we had a few timeouts.
@dratwas as far as I can tell the differences in the approach here, are mainly about what level this should occur at (it looks like we prefer this at a lower level) and the abstraction level, eg should a block be aware of an implementation detail like block id, vs telling rich text if it's the original block or not.
cc @TimothyBJacobs in case you wanted to take this for a spin.
ellatrix
force-pushed
the
try/spilt-keep-id
branch
from
f187e62
to
f5e3086
Compare
This was referenced Mar 2, 2021
3 tasks
Closed
7 tasks
fluiddot
added a commit
that referenced
this pull request
Mar 12, 2021
* Release script: Update react-native-editor version to 1.47.0 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Ignore column width attribute when empty (#29015) * Ignore empty width attribute in columns The editor was crashing when attempting to render a column block which contained an empty width attribute (i.e `<!-- wp:column {"width":""} -->`. The exception occurred when accessing a property of an undefined object, so adding a default object (`|| {}`) fixes this by allowing the property to be accessed. * Update react-native-editor changelog * Add missing dependency to useSelect in columns block Co-authored-by: Carlos Garcia <fluiddot@gmail.com> * Update editor changelog * Mobile - RichText - Restore onSelectionChange when its focused (#29074) * Adds null check before function call * [RNMobile] Merge 1.46.1 beta fix release to 1.47.0 (#29044) * Release script: Update react-native-editor version to 1.46.1 * Release script: Update with changes from 'npm run core preios' * Update react-native-editor CHANGELOG * [RNMobile] Add capability to bridge to show/hide audio block (#28952) * [RNMobile] Ignore column width attribute when empty (#29015) * Update react-native-editor CHANGELOG * Fixes minor changelog alignement issue * Add Stories bridge methods for iOS (#29083) Add missing bridge methods for the Stories block. The absence of these threw an error, even though Stories block isn't fully supported for iOS just yet. Co-authored-by: Ceyhun Ozugur <ceyhunozugur@gmail.com> Co-authored-by: Paul Von Schrottky <paul.von.schrottky@automattic.com> Co-authored-by: Antonis Lilis <antonis.lilis@automattic.com> Co-authored-by: David Calhoun <dpcalhoun@gmail.com> * Release script: Update react-native-editor version to 1.48.0 * Release script: Update with changes from 'npm run core preios' * [Mobile] - Fix splitting/merging of Paragraph and Heading (#29502) * Wip: Mobile RichText - Updating old value after splitting * Mobile - Fix splitting/merging issues and keyboard jumpiness on Android * Mobile - RichText - Add isIOS check for componentDidUpdate and use blockEditorStore * Mobile - RichText - Prevent onTextUpdate on Android * Update changelog * Changelog - fix typo * Revert "[Mobile] - Fix splitting/merging of Paragraph and Heading (#29502)" This reverts commit a14915f. * Revert "Rich text: keep block ID on split (#28505)" This reverts commit 4b9d13f. * Release script: Update react-native-editor version to 1.47.1 * Release script: Update with changes from 'npm run core preios' * Change the maximum items per page of reusable block fetch * Release script: Update react-native-editor version to 1.48.1 * Release script: Update with changes from 'npm run core preios' * Change the maximum items per page of reusable block fetch * Add replace block content by clientID * Add item to release notes about `replaceBlock` method * Revert "Revert "Rich text: keep block ID on split (#28505)"" This reverts commit 956cdfc. Co-authored-by: Antonis Lilis <antonis.lilis@automattic.com> Co-authored-by: Paul Von Schrottky <paul.von.schrottky@automattic.com> Co-authored-by: David Calhoun <dpcalhoun@gmail.com> Co-authored-by: Gerardo Pacheco <gerardo.pacheco@automattic.com> Co-authored-by: Ceyhun Ozugur <ceyhunozugur@gmail.com> Co-authored-by: Enej Bajgoric <enej.bajgoric@automattic.com> Co-authored-by: Brandon Titus <b@titus.io>
chipsnyder
added a commit
that referenced
this pull request
Mar 16, 2021
This reverts commit 4b9d13f.
chipsnyder
added a commit
that referenced
this pull request
Mar 18, 2021
This reverts commit 9fcf73f.
mkevins
added a commit
that referenced
this pull request
Mar 19, 2021
* Release script: Update react-native-editor version to 1.49.0 * Release script: Update with changes from 'npm run core preios' * Revert "Rich text: keep block ID on split (#28505)" This reverts commit 4b9d13f. * Update Changelog * Revert "Revert "Rich text: keep block ID on split (#28505)"" This reverts commit 9fcf73f. Co-authored-by: Chip Snyder <chip.snyder3@gmail.com>
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.
Description
This change ensures the block ID is kept for one of the split blocks, which is usually the first value unless it is empty and the second value is non-empty.
This also fixes an issue with the heading block where two paragraphs would be created if you split an empty heading in two. Instead, it should be a heading and a paragraph.
How has this been tested?
Screenshots
Types of changes
Checklist: