Skip to content
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.

Sign up for GitHub

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

Preview: skip rendering rich text #60544

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Preview: skip rendering rich text #60544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
476241d
Preview: skip rendering rich text
ellatrix Apr 7, 2024
20f21ce
Correction
ellatrix Apr 7, 2024
4700624
Avoid Content component
ellatrix Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/components/block-edit/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const mayDisplayControlsKey = Symbol( 'mayDisplayControls' );
export const mayDisplayParentControlsKey = Symbol( 'mayDisplayParentControls' );
export const blockEditingModeKey = Symbol( 'blockEditingMode' );
export const blockBindingsKey = Symbol( 'blockBindings' );
export const isPreviewModeKey = Symbol( 'isPreviewMode' );

export const DEFAULT_BLOCK_EDIT_CONTEXT = {
name: '',
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/block-edit/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
mayDisplayParentControlsKey,
blockEditingModeKey,
blockBindingsKey,
isPreviewModeKey,
} from './context';

/**
Expand All @@ -31,6 +32,7 @@ export default function BlockEdit( {
mayDisplayControls,
mayDisplayParentControls,
blockEditingMode,
isPreviewMode,
// The remaining props are passed through the BlockEdit filters and are thus
// public API!
...props
Expand Down Expand Up @@ -65,6 +67,7 @@ export default function BlockEdit( {
[ mayDisplayParentControlsKey ]: mayDisplayParentControls,
[ blockEditingModeKey ]: blockEditingMode,
[ blockBindingsKey ]: bindings,
[ isPreviewModeKey ]: isPreviewMode,
} ),
[
name,
Expand All @@ -77,6 +80,7 @@ export default function BlockEdit( {
mayDisplayParentControls,
blockEditingMode,
bindings,
isPreviewMode,
]
) }
>
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ function BlockListBlock( {
mayDisplayControls={ mayDisplayControls }
mayDisplayParentControls={ mayDisplayParentControls }
blockEditingMode={ context.blockEditingMode }
isPreviewMode={ context.isPreviewMode }
/>
);

Expand Down Expand Up @@ -572,6 +573,7 @@ function BlockListBlockProvider( props ) {
} = getSettings();
const hasLightBlockWrapper = blockType?.apiVersion > 1;
const previewContext = {
isPreviewMode,
blockWithoutAttributes,
name: blockName,
attributes,
Expand Down Expand Up @@ -671,6 +673,7 @@ function BlockListBlockProvider( props ) {
);

const {
isPreviewMode,
// Fill values that end up as a public API and may not be defined in
// preview mode.
mode = 'visual',
Expand Down Expand Up @@ -728,6 +731,7 @@ function BlockListBlockProvider( props ) {
}

const privateContext = {
isPreviewMode,
clientId,
className,
index,
Expand Down
45 changes: 26 additions & 19 deletions packages/block-editor/src/components/rich-text/content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,40 @@ import RichText from './';
*/
import { getMultilineTag } from './utils';

export function Content( {
value,
tagName: Tag,
multiline,
format,
...props
} ) {
export function valueToHTMLString( value, multiline ) {
if ( RichText.isEmpty( value ) ) {
const MultilineTag = getMultilineTag( multiline );
value = MultilineTag ? <MultilineTag /> : null;
} else if ( Array.isArray( value ) ) {
const multilineTag = getMultilineTag( multiline );
return multilineTag ? `<${ multilineTag }></${ multilineTag }>` : '';
}

if ( Array.isArray( value ) ) {
deprecated( 'wp.blockEditor.RichText value prop as children type', {
since: '6.1',
version: '6.3',
alternative: 'value prop as string',
link: 'https://developer.wordpress.org/block-editor/how-to-guides/block-tutorial/introducing-attributes-and-editable-fields/',
} );
value = <RawHTML>{ childrenSource.toHTML( value ) }</RawHTML>;
} else if ( typeof value === 'string' ) {
// To do: deprecate.
value = <RawHTML>{ value }</RawHTML>;
} else {
// To do: create a toReactComponent method on RichTextData, which we
// might in the future also use for the editable tree. See
// https://github.com/WordPress/gutenberg/pull/41655.
value = <RawHTML>{ value.toHTMLString() }</RawHTML>;
return childrenSource.toHTML( value );
}

// To do: deprecate string type.
if ( typeof value === 'string' ) {
return value;
}

// To do: create a toReactComponent method on RichTextData, which we
// might in the future also use for the editable tree. See
// https://github.com/WordPress/gutenberg/pull/41655.
return value.toHTMLString();
}

export function Content( {
value,
tagName: Tag,
multiline,
format,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ellatrix, is the format prop used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a very old prop. I believe years ago at the start of GB we had string and children. We don't check it anymore, instead we rely on the type of the value (string or array). But we also shouldn't remove this because we'd be spreading it on a plain tag. Maybe it's ok because it's just a React warning in dev mode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it; it seems fine to keep it around.

There are no warnings. I was looking into React Native code and noticed this prop, but I wasn't sure if it was used anywhere.

...props
} ) {
value = <RawHTML>{ valueToHTMLString( value, multiline ) }</RawHTML>;
return Tag ? <Tag { ...props }>{ value }</Tag> : value;
}
48 changes: 46 additions & 2 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { getBlockType, store as blocksStore } from '@wordpress/blocks';
*/
import { useBlockEditorAutocompleteProps } from '../autocomplete';
import { useBlockEditContext } from '../block-edit';
import { blockBindingsKey } from '../block-edit/context';
import { blockBindingsKey, isPreviewModeKey } from '../block-edit/context';
import FormatToolbarContainer from './format-toolbar-container';
import { store as blockEditorStore } from '../../store';
import { useUndoAutomaticChange } from './use-undo-automatic-change';
Expand All @@ -44,7 +44,7 @@ import { useInsertReplacementText } from './use-insert-replacement-text';
import { useFirefoxCompat } from './use-firefox-compat';
import FormatEdit from './format-edit';
import { getAllowedFormats } from './utils';
import { Content } from './content';
import { Content, valueToHTMLString } from './content';
import { withDeprecations } from './with-deprecations';
import { unlock } from '../../lock-unlock';
import { canBindBlock } from '../../hooks/use-bindings-attributes';
Expand Down Expand Up @@ -485,6 +485,50 @@ PrivateRichText.isEmpty = ( value ) => {
* @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/block-editor/src/components/rich-text/README.md
*/
const PublicForwardedRichTextContainer = forwardRef( ( props, ref ) => {
const context = useBlockEditContext();
const isPreviewMode = context[ isPreviewModeKey ];

if ( isPreviewMode ) {
// Remove all non-content props.
const {
children,
tagName: Tag = 'div',
value,
onChange,
isSelected,
multiline,
inlineToolbar,
wrapperClassName,
autocompleters,
onReplace,
placeholder,
allowedFormats,
withoutInteractiveFormatting,
onRemove,
onMerge,
onSplit,
__unstableOnSplitAtEnd,
__unstableOnSplitAtDoubleLineEnd,
identifier,
preserveWhiteSpace,
__unstablePastePlainText,
__unstableEmbedURLOnPaste,
__unstableDisableFormats,
disableLineBreaks,
__unstableAllowPrefixTransformations,
readOnly,
...contentProps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if using "opt-in" (listing the content props and omitting the rest) is clearer than "opt-out" here. Also why remove "native props" from preview, aren't these "style related" previews that are important for previews?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do that because we don't know all the possible attributes people could be passing to the tag. I mean, we can't unless we list all possible React props.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok but what about this part "aren't these "style related" previews that are important for previews?"

in other words, why are we calling removeNativeProps here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block authors can pass all sorts of attributes to rich text, it's not just styles. Classes, data attributes, and all other valid HTML attribute names. We are removing native props because these are component props, not attribute names, and otherwise React will throw warnings that these aren't valid attribute names when trying to set them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I got confused by the "native" in the function name. I thought it was about removing regular DOM attributes rather than removing "native like mobile native" RichText version attribute.

} = removeNativeProps( props );
return (
<Tag
{ ...contentProps }
dangerouslySetInnerHTML={ {
__html: valueToHTMLString( value, multiline ),
} }
/>
);
}

return <PrivateRichText ref={ ref } { ...props } readOnly={ false } />;
} );

Expand Down
Loading