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

Limit start value on ordered lists #47362

Closed
wants to merge 3 commits into from
Closed

Limit start value on ordered lists #47362

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
42ebb6c
Limit start value on ordered lists
jhnstn Jan 23, 2023
be5a839
Revert integer check on rich text component
jhnstn Mar 10, 2023
a45068e
Clean up integer limit function
jhnstn Mar 10, 2023
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
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/rich-text/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,14 @@ import EmbedHandlerPicker from './embed-handler-picker';

const classes = 'block-editor-rich-text__editable';

function limitIntegerValue( value ) {
twstokes marked this conversation as resolved.
Show resolved Hide resolved
const int32bit = Math.pow( 2, 31 );
return Math.min(
Math.max( parseInt( value, 10 ), -1 * ( int32bit - 1 ) ),
int32bit - 1
);
}

function RichTextWrapper(
{
children,
Expand Down Expand Up @@ -589,7 +597,7 @@ function RichTextWrapper(
selectionEnd={ selectionEnd }
onSelectionChange={ onSelectionChange }
tagName={ tagName }
start={ start }
start={ limitIntegerValue( start ) }
reversed={ reversed }
placeholder={ placeholder }
allowedFormats={ adjustedAllowedFormats }
Expand Down
53 changes: 53 additions & 0 deletions packages/block-library/src/list/ordered-list-settings.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { InspectorControls } from '@wordpress/block-editor';
import { TextControl, PanelBody, ToggleControl } from '@wordpress/components';

function limitIntegerValue( value ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're not taking in a min / max and it's specific to signed 32-bit integers, should we make the function name more specific, or add a comment describing what it does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a different name for the function in an earlier PR a bit more specific and it was suggested that it be named limitValue. I thought that it was too generic so opted for limitIntegerValue to imply that the max min limits were integer limits. But I see your point about the 32 bit integer assumption which is not explicit in the function name. Perhaps a comment is enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO a comment is fine if we're keeping it local to these files.

const int = parseInt( value, 10 );
if ( isNaN( int ) ) {
return undefined;
}
const int32bit = Math.pow( 2, 31 );
return Math.min(
Math.max( parseInt( value, 10 ), -1 * ( int32bit - 1 ) ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could the const int be reused here instead of parsing the value again?

int32bit - 1
);
}

const OrderedListSettings = ( { setAttributes, reversed, start } ) => (
<InspectorControls>
<PanelBody title={ __( 'Ordered list settings' ) }>
<TextControl
__nextHasNoMarginBottom
label={ __( 'Start value' ) }
type="number"
onChange={ ( value ) => {
const int = limitIntegerValue( value );

setAttributes( {
// It should be possible to unset the value,
// e.g. with an empty string.
start: isNaN( int ) ? undefined : int,
} );
} }
value={ Number.isInteger( start ) ? start.toString( 10 ) : '' }
step="1"
/>
<ToggleControl
label={ __( 'Reverse list numbering' ) }
checked={ reversed || false }
onChange={ ( value ) => {
setAttributes( {
// Unset the attribute if not reversed.
reversed: value || undefined,
} );
} }
/>
</PanelBody>
</InspectorControls>
);

export default OrderedListSettings;