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

[RNMobile] Update image size UI control #19232

Merged
merged 12 commits into from
Feb 12, 2020
Merged

[RNMobile] Update image size UI control #19232

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
95e7430
SelectControl on mobile updated to CyclePicker UI
cameronvoell Dec 19, 2019
c70193b
Image size options now from editor settings store
cameronvoell Dec 19, 2019
82ab9d3
Make CyclePickerCell variable name generic
cameronvoell Dec 19, 2019
31caac7
New CycleSelectControl added
cameronvoell Jan 15, 2020
c4be661
Merge branch 'master' into rnmobile/fix-image-size-ui-control
cameronvoell Jan 15, 2020
697ab01
Merge branch 'master' into rnmobile/fix-image-size-ui-control
cameronvoell Feb 3, 2020
9e71910
Replaced unneccessary getImageSizeOptions function
cameronvoell Feb 3, 2020
9aa8aa7
Adjusted function name and lines for readability
cameronvoell Feb 3, 2020
12ebc85
Added checks for empty size options
cameronvoell Feb 3, 2020
559fb21
Add undefined prop protection
cameronvoell Feb 3, 2020
d6be598
Merge branch 'master' into rnmobile/fix-image-size-ui-control
cameronvoell Feb 12, 2020
34820b3
Incorporating imageSizes defaults change from master
cameronvoell Feb 12, 2020
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
33 changes: 16 additions & 17 deletions packages/block-library/src/image/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,9 @@ import { getUpdatedLinkTargetSettings } from './utils';
import {
LINK_DESTINATION_CUSTOM,
LINK_DESTINATION_NONE,
DEFAULT_SIZE_SLUG,
} from './constants';

const IMAGE_SIZE_THUMBNAIL = 'thumbnail';
const IMAGE_SIZE_MEDIUM = 'medium';
const IMAGE_SIZE_LARGE = 'large';
const IMAGE_SIZE_FULL_SIZE = 'full';
const DEFAULT_SIZE_SLUG = IMAGE_SIZE_LARGE;
const sizeOptionLabels = {
[ IMAGE_SIZE_THUMBNAIL ]: __( 'Thumbnail' ),
[ IMAGE_SIZE_MEDIUM ]: __( 'Medium' ),
[ IMAGE_SIZE_LARGE ]: __( 'Large' ),
[ IMAGE_SIZE_FULL_SIZE ]: __( 'Full Size' ),
};
const sizeOptions = map( sizeOptionLabels, ( label, option ) => ( { value: option, label } ) );

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

// Default Image ratio 4:3
const IMAGE_ASPECT_RATIO = 4 / 3;

Expand Down Expand Up @@ -296,11 +284,18 @@ export class ImageEdit extends React.Component {
}

render() {
const { attributes, isSelected, image } = this.props;
const { attributes, isSelected, image, imageSizes } = this.props;
const { align, url, height, width, alt, href, id, linkTarget, sizeSlug } = attributes;

const actions = [ { label: __( 'Clear All Settings' ), onPress: this.onClearSettings } ];

const getImageSizeOptions = () => map( imageSizes, ( { label, slug } ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's necessary to make that a function here, just computing size options should be fine:

const sizeOptions = imageSizes.map( ( { label, slug } ) => ( { value: slug, label } ) );

Copy link
Member Author

@cameronvoell cameronvoell Feb 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good call, fixed here: 9e71910

return {
value: slug,
label,
};
} );

const getToolbarEditButton = ( open ) => (
<BlockControls>
<ToolbarGroup>
Expand Down Expand Up @@ -340,12 +335,11 @@ export class ImageEdit extends React.Component {
{ // eslint-disable-next-line no-undef
image && __DEV__ &&
<SelectControl
hideCancelButton
icon={ 'editor-expand' }
label={ __( 'Size' ) }
value={ sizeOptionLabels[ sizeSlug || DEFAULT_SIZE_SLUG ] }
value={ sizeSlug || DEFAULT_SIZE_SLUG }
onChangeValue={ ( newValue ) => this.onSetSizeSlug( newValue ) }
options={ sizeOptions }
options={ getImageSizeOptions() }
/> }
<TextControl
icon={ 'editor-textcolor' }
Expand Down Expand Up @@ -484,10 +478,15 @@ export class ImageEdit extends React.Component {
export default compose( [
withSelect( ( select, props ) => {
const { getMedia } = select( 'core' );
const { getSettings } = select( 'core/block-editor' );
const { attributes: { id }, isSelected } = props;
const {
imageSizes,
} = getSettings();

return {
image: id && isSelected ? getMedia( id ) : null,
imageSizes,
};
} ),
withPreferredColorScheme,
Expand Down
30 changes: 30 additions & 0 deletions packages/components/src/mobile/bottom-sheet/cycle-picker-cell.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* External dependencies
*/
import { findIndex } from 'lodash';
/**
* Internal dependencies
*/
import Cell from './cell';

export default function BottomSheetCyclePickerCell( props ) {
const {
value,
options,
onChangeValue,
...cellProps
} = props;

const cycleOptionValue = () => {
Copy link
Contributor

@Tug Tug Jan 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

A better name for this function imo would be nextOptionValue. Cycle does indicate that the possible options form a cycle but it's hard to understand that this is returning the next one from the currently selected one.
I think readability could be improved here as well, something as simple as this would really help I think

const nextOptionValue = () => {
	const selectedOptionIndex = findIndex( options, { value } );
	return options[ ( selectedOptionIndex + 1 ) % options.length ].value;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to handle the case where options is en empty array as well ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated constant name 9aa8aa7 . Added checks here 12ebc85 so the image block will hide image size options if sizes / default are ever empty or not matching, and a check so that the CycleSelectControl value will just be blank if it is ever passed empty options.

return options[ ( findIndex( options, [ 'value', value ] ) + 1 ) % options.length ].value;
};

return (
<Cell
onPress={ () => onChangeValue( cycleOptionValue() ) }
editable={ false }
value={ options[ findIndex( options, [ 'value', value ] ) ].label }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, let's create variables with explicit names that should help improve readability.

Copy link
Member Author

Choose a reason for hiding this comment

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

See comment above, this was improved in 9aa8aa7 and 12ebc85

{ ...cellProps }
/>
);
}
4 changes: 2 additions & 2 deletions packages/components/src/mobile/bottom-sheet/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { withPreferredColorScheme } from '@wordpress/compose';
import styles from './styles.scss';
import Button from './button';
import Cell from './cell';
import PickerCell from './picker-cell';
import CyclePickerCell from './cycle-picker-cell';
import SwitchCell from './switch-cell';
import RangeCell from './range-cell';
import KeyboardAvoidingView from './keyboard-avoiding-view';
Expand Down Expand Up @@ -147,7 +147,7 @@ const ThemedBottomSheet = withPreferredColorScheme( BottomSheet );
ThemedBottomSheet.getWidth = getWidth;
ThemedBottomSheet.Button = Button;
ThemedBottomSheet.Cell = Cell;
ThemedBottomSheet.PickerCell = PickerCell;
ThemedBottomSheet.CyclePickerCell = CyclePickerCell;
ThemedBottomSheet.SwitchCell = SwitchCell;
ThemedBottomSheet.RangeCell = RangeCell;

Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/select-control/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* Internal dependencies
*/
import PickerCell from '../mobile/bottom-sheet/picker-cell';
import CyclePickerCell from '../mobile/bottom-sheet/cycle-picker-cell';

function SelectControl( {
help,
Expand All @@ -17,7 +17,7 @@ function SelectControl( {
const id = `inspector-select-control-${ instanceId }`;

return (
<PickerCell
<CyclePickerCell
Copy link
Member Author

Choose a reason for hiding this comment

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

Though I did not include it in this PR yet. I am proposing that we remove PickerCell.(though Picker is still super useful!). Though PickerCell gets the BottomSheet nested menu about 80% there, it feels like a fundamental restriction of a react-native-modal Modal nested inside of another, that we will not be able to close the containing Modal before showing the child Modal without editing the underlying library. @Tug @etoledom thoughts?

One reason this might be worth doing now is in order to decouple PickerCell from Cross Platform component refactoring efforts, like seen here in SelectControl.

label={ label }
hideLabelFromVision={ hideLabelFromVision }
id={ id }
Expand Down