-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resyncs RichText mobile components with web counterparts. #17897
Changes from 9 commits
ac7c363
679f006
f52937c
fe59886
289bdb8
f269ea5
1fda754
2491bee
14a37cf
b2ec21f
ca1d308
cd86c9c
f3f71d4
51dec2a
d8e7807
c5167db
53194ff
d03b619
8fd2d6c
118d228
540ccd7
fdaaff0
fa4c573
07747d6
9692574
345e655
cc9dab1
1b72134
28ef938
ce481aa
014c931
2cc0087
cf5ee3a
b31067c
b5b433c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createBlobURL } from '@wordpress/blob'; | ||
|
||
export function filePasteHandler(images, html) { | ||
if (!html && images.length > 0) { | ||
let filesHTML = ''; | ||
images.forEach((image) => { | ||
const file = image.getAsFile ? image.getAsFile() : image; | ||
window.console.log('Received item:\n\n', file); | ||
filesHTML += `<img src="${createBlobURL(file)}">` | ||
}); | ||
return filesHTML; | ||
} else { | ||
return undefined | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { createBlobURL } from '@wordpress/blob'; | ||
|
||
SergioEstevao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export function filePasteHandler(images, html) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm trying to understand the differences with the web on this one, and what I'm seeing:
Am I missing extra context, or could these two merge into a single implementation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think the reason the upload happens is because we set this wp-image-uploadId or else the image block will just refer to a local file. No? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super familiar with the whole process, but what I see on the image block is that it will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you know the rational behind this? Why can't we have both? If we want to do this on the native side too, we will need to check for empty string because our paste code sends an empty HTML string when no HTML is available: https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/react-native-aztec/ios/RNTAztecView/RCTAztecView.swift#L245 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like it was a fix for one of those “fun” bugs #4854 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think an empty string would evaluate as false, so the current check should work |
||
if (images && images.length > 0) { | ||
// Allows us to ask for this information when we get a report. | ||
window.console.log( 'Received items:\n\n', images ); | ||
const uploadId = Number.MAX_SAFE_INTEGER; | ||
let filesHTML = ''; | ||
images.forEach((file) => { | ||
filesHTML += `<img src="${file}" class="wp-image-${uploadId}">`; | ||
}); | ||
return filesHTML; | ||
} else { | ||
return undefined; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,9 @@ import { omit } from 'lodash'; | |
/** | ||
* WordPress dependencies | ||
*/ | ||
import { RawHTML, Component, createRef } from '@wordpress/element'; | ||
import { RawHTML, Component, createRef, Platform } from '@wordpress/element'; | ||
import { withDispatch, withSelect } from '@wordpress/data'; | ||
import { pasteHandler, children as childrenSource, getBlockTransforms, findTransform } from '@wordpress/blocks'; | ||
import { pasteHandler, children as childrenSource, getBlockTransforms, findTransform, isUnmodifiedDefaultBlock } from '@wordpress/blocks'; | ||
import { withInstanceId, compose } from '@wordpress/compose'; | ||
import { | ||
__experimentalRichText as RichText, | ||
|
@@ -26,7 +26,6 @@ import { | |
slice, | ||
} from '@wordpress/rich-text'; | ||
import { withFilters, Popover } from '@wordpress/components'; | ||
import { createBlobURL } from '@wordpress/blob'; | ||
import deprecated from '@wordpress/deprecated'; | ||
import { isURL } from '@wordpress/url'; | ||
|
||
|
@@ -38,6 +37,7 @@ import BlockFormatControls from '../block-format-controls'; | |
import FormatToolbar from './format-toolbar'; | ||
import { withBlockEditContext } from '../block-edit/context'; | ||
import { RemoveBrowserShortcuts } from './remove-browser-shortcuts'; | ||
import { filePasteHandler } from './file-paste-handler'; | ||
|
||
const wrapperClasses = 'editor-rich-text block-editor-rich-text'; | ||
const classes = 'editor-rich-text__editable block-editor-rich-text__editable'; | ||
|
@@ -124,7 +124,7 @@ class RichTextWrapper extends Component { | |
} | ||
} | ||
|
||
onPaste( { value, onChange, html, plainText, image } ) { | ||
onPaste( { value, onChange, html, plainText, images } ) { | ||
const { | ||
onReplace, | ||
onSplit, | ||
|
@@ -134,16 +134,13 @@ class RichTextWrapper extends Component { | |
__unstableEmbedURLOnPaste, | ||
} = this.props; | ||
|
||
if ( image && ! html ) { | ||
const file = image.getAsFile ? image.getAsFile() : image; | ||
let imagesHTML = filePasteHandler(images, html) | ||
if ( imagesHTML ) { | ||
const content = pasteHandler( { | ||
HTML: `<img src="${ createBlobURL( file ) }">`, | ||
HTML: imagesHTML, | ||
mode: 'BLOCKS', | ||
tagName, | ||
} ); | ||
|
||
// Allows us to ask for this information when we get a report. | ||
window.console.log( 'Received item:\n\n', file ); | ||
} ); | ||
|
||
if ( onReplace && isEmpty( value ) ) { | ||
onReplace( content ); | ||
|
@@ -429,7 +426,7 @@ class RichTextWrapper extends Component { | |
<FormatToolbar /> | ||
</BlockFormatControls> | ||
) } | ||
{ isSelected && inlineToolbar && hasFormats && ( | ||
{ Platform.OS === 'web' && isSelected && inlineToolbar && hasFormats && ( | ||
SergioEstevao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<Popover | ||
noArrow | ||
position="top center" | ||
|
@@ -440,25 +437,27 @@ class RichTextWrapper extends Component { | |
<FormatToolbar /> | ||
</Popover> | ||
) } | ||
{ isSelected && <RemoveBrowserShortcuts /> } | ||
<Autocomplete | ||
onReplace={ onReplace } | ||
completers={ autocompleters } | ||
record={ value } | ||
onChange={ onChange } | ||
isSelected={ isSelected } | ||
> | ||
{ ( { listBoxId, activeId, onKeyDown } ) => | ||
<Editable | ||
aria-autocomplete={ listBoxId ? 'list' : undefined } | ||
aria-owns={ listBoxId } | ||
aria-activedescendant={ activeId } | ||
start={ start } | ||
reversed={ reversed } | ||
onKeyDown={ onKeyDown } | ||
/> | ||
} | ||
</Autocomplete> | ||
{ Platform.OS === 'web' && isSelected && <RemoveBrowserShortcuts /> } | ||
{ Platform.OS === 'web' && ( | ||
SergioEstevao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
<Autocomplete | ||
onReplace={onReplace} | ||
completers={autocompleters} | ||
record={value} | ||
onChange={onChange} | ||
isSelected={isSelected} | ||
> | ||
{({ listBoxId, activeId, onKeyDown }) => | ||
<Editable | ||
aria-autocomplete={listBoxId ? 'list' : undefined} | ||
aria-owns={listBoxId} | ||
aria-activedescendant={activeId} | ||
start={start} | ||
reversed={reversed} | ||
onKeyDown={onKeyDown} | ||
/> | ||
} | ||
</Autocomplete> | ||
) } | ||
</> | ||
} | ||
</RichText> | ||
|
@@ -482,19 +481,31 @@ class RichTextWrapper extends Component { | |
|
||
const RichTextContainer = compose( [ | ||
withInstanceId, | ||
withBlockEditContext( ( { clientId } ) => ( { clientId } ) ), | ||
withBlockEditContext(({ clientId, onCaretVerticalPositionChange, isSelected }, ownProps) => { | ||
if ( Platform.OS === 'web' ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An idea to keep the current |
||
return { clientId } | ||
} else { | ||
return { | ||
clientId, | ||
blockIsSelected: ownProps.isSelected !== undefined ? ownProps.isSelected : isSelected, | ||
onCaretVerticalPositionChange, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming we need these two, but it would be good to leave a comment here explaining the differences and why those are needed, which could make it easier to consolidate in the future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see #13323 that introduced the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And #15878 introduced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should definitely look into it in a followup PR, there might be "cleaner" ways to address those issues |
||
}; | ||
} | ||
} ), | ||
withSelect( ( select, { | ||
clientId, | ||
instanceId, | ||
identifier = instanceId, | ||
isSelected, | ||
blockIsSelected, | ||
} ) => { | ||
const { | ||
isCaretWithinFormattedText, | ||
getSelectionStart, | ||
getSelectionEnd, | ||
getSettings, | ||
didAutomaticChange, | ||
__unstableGetBlockWithoutInnerBlocks, | ||
} = select( 'core/block-editor' ); | ||
|
||
const selectionStart = getSelectionStart(); | ||
|
@@ -509,13 +520,27 @@ const RichTextContainer = compose( [ | |
isSelected = selectionStart.clientId === clientId; | ||
} | ||
|
||
let extraProps = {} | ||
if ( Platform.OS !== 'web' ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two props don't seem to conflict with the web stuff, so maybe we can skip the check and always inject them. Maybe we could have something like a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the check for platform not make it obvious enough? I could change the code to |
||
// If the block of this RichText is unmodified then it's a candidate for replacing when adding a new block. | ||
// In order to fix https://github.com/wordpress-mobile/gutenberg-mobile/issues/1126, let's blur on unmount in that case. | ||
// This apparently assumes functionality the BlockHlder actually | ||
const block = clientId && __unstableGetBlockWithoutInnerBlocks(clientId); | ||
const shouldBlurOnUnmount = block && isSelected && isUnmodifiedDefaultBlock(block); | ||
extraProps = { | ||
blockIsSelected, | ||
SergioEstevao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
shouldBlurOnUnmount, | ||
} | ||
} | ||
|
||
return { | ||
canUserUseUnfilteredHTML: __experimentalCanUserUseUnfilteredHTML, | ||
isCaretWithinFormattedText: isCaretWithinFormattedText(), | ||
selectionStart: isSelected ? selectionStart.offset : undefined, | ||
selectionEnd: isSelected ? selectionEnd.offset : undefined, | ||
isSelected, | ||
didAutomaticChange: didAutomaticChange(), | ||
...extraProps, | ||
}; | ||
} ), | ||
withDispatch( ( dispatch, { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought ESLint would complain about
console.log
statements, but I see it was already there. Maybe that's why it's usingwindow.console
? If that's the case and we want to keep it, using// eslint-disable-next-line no-console
would be more explicit.