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.

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

[RNMobile][Embed block] Block settings #33654

Merged
merged 19 commits into from
Aug 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import useBlockTypeImpressions from './hooks/use-block-type-impressions';

const NON_BLOCK_CATEGORIES = [ 'reusable' ];

const ALLOWED_EMBED_VARIATIONS = [ 'core/embed' ];

function BlockTypesTab( { onSelect, rootClientId, listProps } ) {
const clipboardBlock = useClipboardBlock( rootClientId );

Expand All @@ -22,7 +24,14 @@ function BlockTypesTab( { onSelect, rootClientId, listProps } ) {

const allItems = getInserterItems( rootClientId );
const blockItems = allItems.filter(
( { category } ) => ! NON_BLOCK_CATEGORIES.includes( category )
( { id, category } ) =>
! NON_BLOCK_CATEGORIES.includes( category ) &&
// We don't want to show all possible embed variations
// as different blocks in the inserter. We'll only show a
// few popular ones.
( category !== 'embed' ||
( category === 'embed' &&
ALLOWED_EMBED_VARIATIONS.includes( id ) ) )
Copy link
Member Author

Choose a reason for hiding this comment

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

Normally the embed variations were hidden from the inserter as we had an empty embed/variations.native.js file (which I deleted in this PR). We needed to re-enable those as the information if an embed supports this setting is kept in the attributes there with the responsive flag:

attributes: { providerNameSlug: 'twitter', responsive: true },

This should also help us when we add a few more variations for wordpress-mobile/gutenberg-mobile#3278

);

return {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ describe( 'BlockTypesTab component', () => {
selectMock.getInserterItems.mockReturnValue( items );

const blockItems = items.filter(
( { category } ) => category !== 'reusable'
( { id, category } ) =>
category !== 'reusable' && id !== 'core-embed/a-paragraph-embed'
);
const component = shallow(
<BlockTypesTab
Expand Down
11 changes: 0 additions & 11 deletions packages/block-library/src/embed/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,6 @@ import { useBlockProps } from '@wordpress/block-editor';
import { store as coreStore } from '@wordpress/core-data';
import { View } from '@wordpress/primitives';

function getResponsiveHelp( checked ) {
return checked
? __(
'This embed will preserve its aspect ratio when the browser is resized.'
)
: __(
'This embed may not preserve its aspect ratio when the browser is resized.'
);
}

const EmbedEdit = ( props ) => {
const {
attributes: {
Expand Down Expand Up @@ -242,7 +232,6 @@ const EmbedEdit = ( props ) => {
themeSupportsResponsive={ themeSupportsResponsive }
blockSupportsResponsive={ responsive }
allowResponsive={ allowResponsive }
getResponsiveHelp={ getResponsiveHelp }
toggleResponsive={ toggleResponsive }
switchBackToURLInput={ () => setIsEditingURL( true ) }
/>
Expand Down
45 changes: 43 additions & 2 deletions packages/block-library/src/embed/edit.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/
import {
createUpgradedEmbedBlock,
getClassNames,
getAttributesFromPreview,
getEmbedInfoByProvider,
} from './util';
Expand Down Expand Up @@ -62,12 +63,18 @@ const EmbedEdit = ( props ) => {
isSelected && wasBlockJustInserted && ! url
);

const { preview, fetching, cannotEmbed } = useSelect(
const {
preview,
fetching,
themeSupportsResponsive,
cannotEmbed,
} = useSelect(
( select ) => {
const {
getEmbedPreview,
isPreviewEmbedFallback,
isRequestingEmbedPreview,
getThemeSupports,
} = select( coreStore );
if ( ! url ) {
return { fetching: false, cannotEmbed: false };
Expand Down Expand Up @@ -97,6 +104,9 @@ const EmbedEdit = ( props ) => {
return {
preview: validPreview ? embedPreview : undefined,
fetching: isFetching,
themeSupportsResponsive: getThemeSupports()[
'responsive-embeds'
],
cannotEmbed: ! validPreview || previewIsFallback,
};
},
Expand All @@ -120,6 +130,21 @@ const EmbedEdit = ( props ) => {
};
};

const toggleResponsive = () => {
const { allowResponsive, className } = attributes;
const { html } = preview;
const newAllowResponsive = ! allowResponsive;

setAttributes( {
allowResponsive: newAllowResponsive,
className: getClassNames(
html,
className,
responsive && newAllowResponsive
),
} );
};

useEffect( () => {
if ( ! preview?.html || ! cannotEmbed || fetching ) {
return;
Expand Down Expand Up @@ -167,7 +192,19 @@ const EmbedEdit = ( props ) => {
}

const showEmbedPlaceholder = ! preview || cannotEmbed;
const { type, className: classFromPreview } = getMergedAttributes();

// Even though we set attributes that get derived from the preview,
// we don't access them directly because for the initial render,
// the `setAttributes` call will not have taken effect. If we're
// rendering responsive content, setting the responsive classes
// after the preview has been rendered can result in unwanted
// clipping or scrollbars. The `getAttributesFromPreview` function
// that `getMergedAttributes` uses is memoized so that we're not
const {
type,
allowResponsive,
className: classFromPreview,
} = getMergedAttributes();
const className = classnames( classFromPreview, props.className );

return (
Expand All @@ -189,6 +226,10 @@ const EmbedEdit = ( props ) => {
<>
<EmbedControls
showEditButton={ preview && ! cannotEmbed }
themeSupportsResponsive={ themeSupportsResponsive }
blockSupportsResponsive={ responsive }
allowResponsive={ allowResponsive }
toggleResponsive={ toggleResponsive }
switchBackToURLInput={ () => setIsEditingURL( true ) }
/>
<View { ...blockProps }>
Expand Down
11 changes: 10 additions & 1 deletion packages/block-library/src/embed/embed-controls.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,21 @@ import {
import { BlockControls, InspectorControls } from '@wordpress/block-editor';
import { edit } from '@wordpress/icons';

function getResponsiveHelp( checked ) {
return checked
? __(
'This embed will preserve its aspect ratio when the browser is resized.'
)
: __(
'This embed may not preserve its aspect ratio when the browser is resized.'
);
}
ceyhun marked this conversation as resolved.
Show resolved Hide resolved

const EmbedControls = ( {
blockSupportsResponsive,
showEditButton,
themeSupportsResponsive,
allowResponsive,
getResponsiveHelp,
toggleResponsive,
switchBackToURLInput,
} ) => (
Expand Down
23 changes: 0 additions & 23 deletions packages/block-library/src/embed/embed-controls.native.js

This file was deleted.

7 changes: 2 additions & 5 deletions packages/block-library/src/embed/embed-no-preview.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { useSelect } from '@wordpress/data';
import { store as editorStore } from '@wordpress/editor';
import { BottomSheet, Icon, TextControl } from '@wordpress/components';
import { help } from '@wordpress/icons';
import { BlockIcon } from '@wordpress/block-editor';

/**
* Internal dependencies
Expand All @@ -38,10 +39,6 @@ const EmbedNoPreview = ( { label, icon, isSelected, onPress } ) => {
styles.embed__label,
styles[ 'embed__label--dark' ]
);
const embedIconStyle = usePreferredColorSchemeStyle(
styles.embed__icon,
styles[ 'embed__icon--dark' ]
);
const descriptionStyle = usePreferredColorSchemeStyle(
styles.embed__description,
styles[ 'embed__description--dark' ]
Expand Down Expand Up @@ -118,7 +115,7 @@ const EmbedNoPreview = ( { label, icon, isSelected, onPress } ) => {
onPress={ onPressContainer }
>
<View style={ containerStyle }>
<Icon icon={ icon } fill={ embedIconStyle.fill } />
<BlockIcon icon={ icon } />
Copy link
Member Author

Choose a reason for hiding this comment

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

Now that we've enabled all the variations, icons can also be objects:

export const embedTwitterIcon = {
foreground: '#1da1f2',
src: (
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24">
<G>
<Path d="M22.23 5.924c-.736.326-1.527.547-2.357.646.847-.508 1.498-1.312 1.804-2.27-.793.47-1.67.812-2.606.996C18.325 4.498 17.258 4 16.078 4c-2.266 0-4.103 1.837-4.103 4.103 0 .322.036.635.106.935-3.41-.17-6.433-1.804-8.457-4.287-.353.607-.556 1.312-.556 2.064 0 1.424.724 2.68 1.825 3.415-.673-.022-1.305-.207-1.86-.514v.052c0 1.988 1.415 3.647 3.293 4.023-.344.095-.707.145-1.08.145-.265 0-.522-.026-.773-.074.522 1.63 2.038 2.817 3.833 2.85-1.404 1.1-3.174 1.757-5.096 1.757-.332 0-.66-.02-.98-.057 1.816 1.164 3.973 1.843 6.29 1.843 7.547 0 11.675-6.252 11.675-11.675 0-.178-.004-.355-.012-.53.802-.578 1.497-1.3 2.047-2.124z"></Path>
</G>
</SVG>
),
};

BlockIcon component was used in the web version and it handles this case as well.

<Text style={ labelStyle }>{ label }</Text>
<Text style={ descriptionStyle }>
{ __( 'Embed previews not yet available' ) }
Expand Down
7 changes: 2 additions & 5 deletions packages/block-library/src/embed/embed-placeholder.native.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { View, Text, TouchableWithoutFeedback } from 'react-native';
import { __ } from '@wordpress/i18n';
import { usePreferredColorSchemeStyle } from '@wordpress/compose';
import { Icon } from '@wordpress/components';
import { BlockIcon } from '@wordpress/block-editor';

/**
* Internal dependencies
Expand Down Expand Up @@ -37,10 +38,6 @@ const EmbedPlaceholder = ( {
styles.embed__action,
styles[ 'embed__action--dark' ]
);
const embedIconStyle = usePreferredColorSchemeStyle(
styles.embed__icon,
styles[ 'embed__icon--dark' ]
);
const embedIconErrorStyle = styles[ 'embed__icon--error' ];

return (
Expand Down Expand Up @@ -73,7 +70,7 @@ const EmbedPlaceholder = ( {
</>
) : (
<>
<Icon icon={ icon } fill={ embedIconStyle.fill } />
<BlockIcon icon={ icon } />
<Text style={ labelStyle }>{ label }</Text>
<Text style={ actionStyle }>
{ __( 'ADD LINK' ) }
Expand Down
8 changes: 0 additions & 8 deletions packages/block-library/src/embed/styles.native.scss
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,6 @@
background-color: $background-dark-secondary;
}

.embed__icon {
fill: $light-secondary;
}

.embed__icon--dark {
fill: $dark-secondary;
}

.embed__icon--error {
margin-bottom: 6;
fill: $alert-red;
Expand Down
3 changes: 0 additions & 3 deletions packages/block-library/src/embed/variations.native.js

This file was deleted.

10 changes: 8 additions & 2 deletions packages/components/src/toggle-control/index.native.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import { isFunction } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -11,15 +16,16 @@ const ToggleControl = memo(
( { label, checked, help, instanceId, className, onChange, ...props } ) => {
const id = `inspector-toggle-control-${ instanceId }`;

const helpLabel = help && isFunction( help ) ? help( checked ) : help;

return (
<SwitchCell
label={ label }
id={ id }
help={ help }
help={ helpLabel }
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that on Android when tapping over the "Resize for smaller devices" element the ripple animation covers also the help message, not sure if this is expected 🤔 .

Screen_Recording_20210813-153056_WordPress.Pre-Alpha.mp4

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I didn't make changes around that in this PR. I've checked with Audio Block settings and it seems to be the same there. Pressing directly on the toggle doesn't create that effect, so I guess it could be the bottom sheet row being a touchable that's making that happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, looks like this is related to the fact that the help message is rendered within the TouchableRipple component:

{ help && (
<Text style={ [ cellHelpStyle, styles.placeholderColor ] }>
{ help }
</Text>
) }

If we're using it in other places I guess it's expected, thanks for checking it 🙇 .

className={ className }
value={ checked }
onValueChange={ onChange }
aria-describedby={ !! help ? id + '__help' : undefined }
{ ...props }
/>
);
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i
-->

## Unreleased
- [**] Embed block: Add "Resize for smaller devices" setting. [#33654]

## 1.59.1
- [*] Global styles - Add color to the block styles filter list [#34000]
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native-editor/src/api-fetch-setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import apiFetch from '@wordpress/api-fetch';

// Please add only wp.org API paths here!
const SUPPORTED_ENDPOINTS = [
/wp\/v2\/(media|categories|blocks)\/?\d*?.*/i,
/wp\/v2\/(media|categories|blocks|themes)\/?\d*?.*/i,
Copy link
Member Author

Choose a reason for hiding this comment

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

getThemeSupports selector may trigger a request to /wp/v2/themes?status=active

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should fetch theme data via the bridge instead of directly making the API request, for other values like colors or gradients, we're getting them from the editor settings:

private func properties(from editorSettings: GutenbergEditorSettings?) -> [String : Any] {
var settingsUpdates = [String : Any]()
settingsUpdates["isFSETheme"] = editorSettings?.isFSETheme ?? false
if let rawStyles = editorSettings?.rawStyles {
settingsUpdates["rawStyles"] = rawStyles
}
if let rawFeatures = editorSettings?.rawFeatures {
settingsUpdates["rawFeatures"] = rawFeatures
}
if let colors = editorSettings?.colors {
settingsUpdates["colors"] = colors
}
if let gradients = editorSettings?.gradients {
settingsUpdates["gradients"] = gradients
}
return settingsUpdates
}

From my POV it's totally ok to fetch from the editor but we might have issues for supporting offline mode, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried fetching it from the bridge on WPiOS via WordPressKit-iOS, but it seems like the /wp-block-editor/v1/settings endpoint is used instead of /wp/v2/themes if the blog supports it here and the /settings endpoint currently doesn't seem to return the responsive-embeds value we need.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried fetching it from the bridge on WPiOS via WordPressKit-iOS, but it seems like the /wp-block-editor/v1/settings endpoint is used instead of /wp/v2/themes if the blog supports it here and the /settings endpoint currently doesn't seem to return the responsive-embeds value we need.

Right, I checked the schema of /wp-block-editor/v1/settings (reference) and there's no reference to responsive-embeds, so looks like the theme support settings are only included in the theme model. Now I'm wondering if we should continue fetching the theme via the api-fetch or from the theme service (WordPress-iOS reference and WordPressKit-iOS reference) and expose it in the RN bridge, wdyt?

NOTE: I noticed that the Theme model (reference) doesn't include the theme support settings so, in case we go that way, we should include it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the response from /sites/%@/themes/mine also doesn't return the responsive-embeds value. I couldn't find any other endpoint that returns it, but could be that I missed out something.
Otherwise I guess we can still fetch it from /wp/v2/themes via BlockEditorSettingsServiceRemote in WordPressKit-iOS with the function here, even when the blog supports /wp-block-editor/v1/settings endpoint. But I think WPiOS is expecting both endpoints to have the same response structure as they are both handed the same completion handler here. I think this is not the case, but again I may be overlooking something.
This might mean that we may need to create a new model for the /wp/v2/themes response along with its local persistence counterparts, which might require some more effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, I'm afraid that this could turn more complex than what we would expect for this task. At this point, I'd lean towards stick with using the fetch as you originally implemented and open an issue as a follow-up to tackle this in the future, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, opened an issue for this: wordpress-mobile/gutenberg-mobile#3825

/wp\/v2\/search\?.*/i,
/oembed\/1\.0\/proxy\?.*/i,
];
Expand Down