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

Edit in place hook #28924

Closed
wants to merge 22 commits into from
Closed

Edit in place hook #28924

wants to merge 22 commits into from

Conversation

grzim
Copy link
Contributor

@grzim grzim commented Feb 10, 2021

Description

Hook providing with refs to bind two elements.

One ref is used to set 'isEdit' state to true upon clicking. The second is used to set 'isEdit' state to false on 'Enter'/onBlur event and triggering onCommit callback with its value.

This PR included a component in the first place, but the hook gives more flexibility.

How has this been tested?

Tested manually.

Screenshots

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions
Copy link

github-actions bot commented Feb 10, 2021

Size Change: +5.68 kB (0%)

Total Size: 1.38 MB

Filename Size Change
build/annotations/index.js 3.78 kB +3 B (0%)
build/autop/index.js 2.84 kB -1 B (0%)
build/block-directory/index.js 9.1 kB +9 B (0%)
build/block-editor/index.js 124 kB +375 B (0%)
build/block-library/blocks/gallery/editor-rtl.css 689 B +10 B (+1%)
build/block-library/blocks/gallery/editor.css 690 B +11 B (+2%)
build/block-library/blocks/navigation/editor-rtl.css 1.34 kB +10 B (+1%)
build/block-library/blocks/navigation/editor.css 1.34 kB +9 B (+1%)
build/block-library/editor-rtl.css 9.05 kB +15 B (0%)
build/block-library/editor.css 9.04 kB +17 B (0%)
build/block-library/index.js 145 kB +94 B (0%)
build/blocks/index.js 48.3 kB +42 B (0%)
build/components/index.js 273 kB +2.93 kB (+1%)
build/components/style-rtl.css 15.6 kB +17 B (0%)
build/components/style.css 15.6 kB +16 B (0%)
build/compose/index.js 11 kB +46 B (0%)
build/core-data/index.js 16.7 kB -28 B (0%)
build/customize-widgets/index.js 4.08 kB -2 B (0%)
build/data-controls/index.js 830 B +3 B (0%)
build/data/index.js 8.86 kB +4 B (0%)
build/date/index.js 31.8 kB -2 B (0%)
build/deprecated/index.js 768 B -1 B (0%)
build/dom-ready/index.js 571 B +1 B (0%)
build/dom/index.js 4.94 kB +1 B (0%)
build/edit-navigation/index.js 11 kB +496 B (+5%) 🔍
build/edit-navigation/style-rtl.css 1.26 kB +149 B (+13%) ⚠️
build/edit-navigation/style.css 1.25 kB +145 B (+13%) ⚠️
build/edit-post/index.js 307 kB -27 B (0%)
build/edit-post/style-rtl.css 6.81 kB +21 B (0%)
build/edit-post/style.css 6.8 kB +21 B (0%)
build/edit-site/index.js 25.4 kB +564 B (+2%)
build/edit-site/style-rtl.css 4.37 kB +334 B (+8%) 🔍
build/edit-site/style.css 4.37 kB +330 B (+8%) 🔍
build/edit-widgets/index.js 20 kB -5 B (0%)
build/editor/index.js 41.9 kB +52 B (0%)
build/element/index.js 4.61 kB -1 B (0%)
build/format-library/index.js 6.77 kB +5 B (0%)
build/hooks/index.js 2.28 kB +1 B (0%)
build/i18n/index.js 4.01 kB -1 B (0%)
build/is-shallow-equal/index.js 698 B -1 B (0%)
build/keyboard-shortcuts/index.js 2.53 kB +1 B (0%)
build/keycodes/index.js 1.93 kB +4 B (0%)
build/list-reusable-blocks/index.js 3.15 kB -1 B (0%)
build/media-utils/index.js 5.36 kB +4 B (0%)
build/notices/index.js 1.85 kB +3 B (0%)
build/nux/index.js 3.41 kB +2 B (0%)
build/plugins/index.js 2.55 kB +2 B (0%)
build/priority-queue/index.js 790 B -1 B (0%)
build/redux-routine/index.js 2.83 kB -6 B (0%)
build/reusable-blocks/index.js 2.92 kB +2 B (0%)
build/rich-text/index.js 13.4 kB +4 B (0%)
build/server-side-render/index.js 2.77 kB -1 B (0%)
build/shortcode/index.js 1.7 kB +1 B (0%)
build/token-list/index.js 1.27 kB +1 B (0%)
build/url/index.js 3.02 kB +3 B (0%)
build/viewport/index.js 1.85 kB -3 B (0%)
build/wordcount/index.js 1.22 kB +1 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 1.01 kB 0 B
build/block-directory/style.css 1.01 kB 0 B
build/block-editor/style-rtl.css 12.1 kB 0 B
build/block-editor/style.css 12.1 kB 0 B
build/block-library/blocks/archives/editor-rtl.css 61 B 0 B
build/block-library/blocks/archives/editor.css 60 B 0 B
build/block-library/blocks/audio/editor-rtl.css 58 B 0 B
build/block-library/blocks/audio/editor.css 58 B 0 B
build/block-library/blocks/audio/style-rtl.css 103 B 0 B
build/block-library/blocks/audio/style.css 103 B 0 B
build/block-library/blocks/block/editor-rtl.css 161 B 0 B
build/block-library/blocks/block/editor.css 161 B 0 B
build/block-library/blocks/button/editor-rtl.css 475 B 0 B
build/block-library/blocks/button/editor.css 474 B 0 B
build/block-library/blocks/button/style-rtl.css 465 B 0 B
build/block-library/blocks/button/style.css 464 B 0 B
build/block-library/blocks/buttons/editor-rtl.css 233 B 0 B
build/block-library/blocks/buttons/editor.css 233 B 0 B
build/block-library/blocks/buttons/style-rtl.css 303 B 0 B
build/block-library/blocks/buttons/style.css 303 B 0 B
build/block-library/blocks/calendar/style-rtl.css 208 B 0 B
build/block-library/blocks/calendar/style.css 208 B 0 B
build/block-library/blocks/categories/editor-rtl.css 84 B 0 B
build/block-library/blocks/categories/editor.css 83 B 0 B
build/block-library/blocks/categories/style-rtl.css 79 B 0 B
build/block-library/blocks/categories/style.css 79 B 0 B
build/block-library/blocks/code/style-rtl.css 90 B 0 B
build/block-library/blocks/code/style.css 90 B 0 B
build/block-library/blocks/columns/editor-rtl.css 190 B 0 B
build/block-library/blocks/columns/editor.css 190 B 0 B
build/block-library/blocks/columns/style-rtl.css 421 B 0 B
build/block-library/blocks/columns/style.css 421 B 0 B
build/block-library/blocks/cover/editor-rtl.css 390 B 0 B
build/block-library/blocks/cover/editor.css 389 B 0 B
build/block-library/blocks/cover/style-rtl.css 1.25 kB 0 B
build/block-library/blocks/cover/style.css 1.25 kB 0 B
build/block-library/blocks/embed/editor-rtl.css 486 B 0 B
build/block-library/blocks/embed/editor.css 486 B 0 B
build/block-library/blocks/embed/style-rtl.css 396 B 0 B
build/block-library/blocks/embed/style.css 395 B 0 B
build/block-library/blocks/file/editor-rtl.css 199 B 0 B
build/block-library/blocks/file/editor.css 198 B 0 B
build/block-library/blocks/file/style-rtl.css 248 B 0 B
build/block-library/blocks/file/style.css 248 B 0 B
build/block-library/blocks/freeform/editor-rtl.css 2.45 kB 0 B
build/block-library/blocks/freeform/editor.css 2.45 kB 0 B
build/block-library/blocks/gallery/style-rtl.css 1.07 kB 0 B
build/block-library/blocks/gallery/style.css 1.06 kB 0 B
build/block-library/blocks/group/editor-rtl.css 318 B 0 B
build/block-library/blocks/group/editor.css 317 B 0 B
build/block-library/blocks/group/style-rtl.css 57 B 0 B
build/block-library/blocks/group/style.css 57 B 0 B
build/block-library/blocks/heading/editor-rtl.css 129 B 0 B
build/block-library/blocks/heading/editor.css 129 B 0 B
build/block-library/blocks/heading/style-rtl.css 76 B 0 B
build/block-library/blocks/heading/style.css 76 B 0 B
build/block-library/blocks/html/editor-rtl.css 281 B 0 B
build/block-library/blocks/html/editor.css 281 B 0 B
build/block-library/blocks/image/editor-rtl.css 717 B 0 B
build/block-library/blocks/image/editor.css 716 B 0 B
build/block-library/blocks/image/style-rtl.css 477 B 0 B
build/block-library/blocks/image/style.css 478 B 0 B
build/block-library/blocks/latest-comments/editor-rtl.css 159 B 0 B
build/block-library/blocks/latest-comments/editor.css 158 B 0 B
build/block-library/blocks/latest-comments/style-rtl.css 269 B 0 B
build/block-library/blocks/latest-comments/style.css 269 B 0 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B 0 B
build/block-library/blocks/latest-posts/editor.css 137 B 0 B
build/block-library/blocks/latest-posts/style-rtl.css 523 B 0 B
build/block-library/blocks/latest-posts/style.css 522 B 0 B
build/block-library/blocks/list/editor-rtl.css 65 B 0 B
build/block-library/blocks/list/editor.css 65 B 0 B
build/block-library/blocks/list/style-rtl.css 63 B 0 B
build/block-library/blocks/list/style.css 63 B 0 B
build/block-library/blocks/media-text/editor-rtl.css 191 B 0 B
build/block-library/blocks/media-text/editor.css 191 B 0 B
build/block-library/blocks/media-text/style-rtl.css 535 B 0 B
build/block-library/blocks/media-text/style.css 532 B 0 B
build/block-library/blocks/more/editor-rtl.css 434 B 0 B
build/block-library/blocks/more/editor.css 434 B 0 B
build/block-library/blocks/navigation-link/editor-rtl.css 395 B 0 B
build/block-library/blocks/navigation-link/editor.css 397 B 0 B
build/block-library/blocks/navigation-link/style-rtl.css 704 B 0 B
build/block-library/blocks/navigation-link/style.css 702 B 0 B
build/block-library/blocks/navigation/style-rtl.css 195 B 0 B
build/block-library/blocks/navigation/style.css 195 B 0 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B 0 B
build/block-library/blocks/nextpage/editor.css 395 B 0 B
build/block-library/blocks/page-list/editor-rtl.css 214 B 0 B
build/block-library/blocks/page-list/editor.css 214 B 0 B
build/block-library/blocks/page-list/style-rtl.css 527 B 0 B
build/block-library/blocks/page-list/style.css 526 B 0 B
build/block-library/blocks/paragraph/editor-rtl.css 109 B 0 B
build/block-library/blocks/paragraph/editor.css 109 B 0 B
build/block-library/blocks/paragraph/style-rtl.css 273 B 0 B
build/block-library/blocks/paragraph/style.css 273 B 0 B
build/block-library/blocks/post-author/editor-rtl.css 209 B 0 B
build/block-library/blocks/post-author/editor.css 209 B 0 B
build/block-library/blocks/post-author/style-rtl.css 183 B 0 B
build/block-library/blocks/post-author/style.css 184 B 0 B
build/block-library/blocks/post-comments-form/style-rtl.css 250 B 0 B
build/block-library/blocks/post-comments-form/style.css 250 B 0 B
build/block-library/blocks/post-content/editor-rtl.css 139 B 0 B
build/block-library/blocks/post-content/editor.css 139 B 0 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B 0 B
build/block-library/blocks/post-excerpt/editor.css 73 B 0 B
build/block-library/blocks/post-featured-image/editor-rtl.css 338 B 0 B
build/block-library/blocks/post-featured-image/editor.css 338 B 0 B
build/block-library/blocks/post-featured-image/style-rtl.css 100 B 0 B
build/block-library/blocks/post-featured-image/style.css 100 B 0 B
build/block-library/blocks/preformatted/style-rtl.css 63 B 0 B
build/block-library/blocks/preformatted/style.css 63 B 0 B
build/block-library/blocks/pullquote/editor-rtl.css 183 B 0 B
build/block-library/blocks/pullquote/editor.css 183 B 0 B
build/block-library/blocks/pullquote/style-rtl.css 316 B 0 B
build/block-library/blocks/pullquote/style.css 316 B 0 B
build/block-library/blocks/query-loop/editor-rtl.css 90 B 0 B
build/block-library/blocks/query-loop/editor.css 89 B 0 B
build/block-library/blocks/query-loop/style-rtl.css 315 B 0 B
build/block-library/blocks/query-loop/style.css 317 B 0 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B 0 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B 0 B
build/block-library/blocks/query-pagination/editor-rtl.css 270 B 0 B
build/block-library/blocks/query-pagination/editor.css 262 B 0 B
build/block-library/blocks/query-pagination/style-rtl.css 168 B 0 B
build/block-library/blocks/query-pagination/style.css 168 B 0 B
build/block-library/blocks/query/editor-rtl.css 159 B 0 B
build/block-library/blocks/query/editor.css 160 B 0 B
build/block-library/blocks/quote/editor-rtl.css 61 B 0 B
build/block-library/blocks/quote/editor.css 61 B 0 B
build/block-library/blocks/quote/style-rtl.css 169 B 0 B
build/block-library/blocks/quote/style.css 169 B 0 B
build/block-library/blocks/rss/editor-rtl.css 201 B 0 B
build/block-library/blocks/rss/editor.css 202 B 0 B
build/block-library/blocks/rss/style-rtl.css 290 B 0 B
build/block-library/blocks/rss/style.css 290 B 0 B
build/block-library/blocks/search/editor-rtl.css 165 B 0 B
build/block-library/blocks/search/editor.css 165 B 0 B
build/block-library/blocks/search/style-rtl.css 342 B 0 B
build/block-library/blocks/search/style.css 344 B 0 B
build/block-library/blocks/separator/editor-rtl.css 99 B 0 B
build/block-library/blocks/separator/editor.css 99 B 0 B
build/block-library/blocks/separator/style-rtl.css 236 B 0 B
build/block-library/blocks/separator/style.css 236 B 0 B
build/block-library/blocks/shortcode/editor-rtl.css 504 B 0 B
build/block-library/blocks/shortcode/editor.css 504 B 0 B
build/block-library/blocks/site-logo/editor-rtl.css 201 B 0 B
build/block-library/blocks/site-logo/editor.css 201 B 0 B
build/block-library/blocks/site-logo/style-rtl.css 117 B 0 B
build/block-library/blocks/site-logo/style.css 117 B 0 B
build/block-library/blocks/social-link/editor-rtl.css 164 B 0 B
build/block-library/blocks/social-link/editor.css 165 B 0 B
build/block-library/blocks/social-links/editor-rtl.css 696 B 0 B
build/block-library/blocks/social-links/editor.css 696 B 0 B
build/block-library/blocks/social-links/style-rtl.css 1.37 kB 0 B
build/block-library/blocks/social-links/style.css 1.37 kB 0 B
build/block-library/blocks/spacer/editor-rtl.css 302 B 0 B
build/block-library/blocks/spacer/editor.css 302 B 0 B
build/block-library/blocks/spacer/style-rtl.css 48 B 0 B
build/block-library/blocks/spacer/style.css 48 B 0 B
build/block-library/blocks/subhead/editor-rtl.css 99 B 0 B
build/block-library/blocks/subhead/editor.css 99 B 0 B
build/block-library/blocks/subhead/style-rtl.css 80 B 0 B
build/block-library/blocks/subhead/style.css 80 B 0 B
build/block-library/blocks/table/editor-rtl.css 489 B 0 B
build/block-library/blocks/table/editor.css 489 B 0 B
build/block-library/blocks/table/style-rtl.css 386 B 0 B
build/block-library/blocks/table/style.css 386 B 0 B
build/block-library/blocks/tag-cloud/editor-rtl.css 118 B 0 B
build/block-library/blocks/tag-cloud/editor.css 118 B 0 B
build/block-library/blocks/tag-cloud/style-rtl.css 94 B 0 B
build/block-library/blocks/tag-cloud/style.css 94 B 0 B
build/block-library/blocks/template-part/editor-rtl.css 557 B 0 B
build/block-library/blocks/template-part/editor.css 556 B 0 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B 0 B
build/block-library/blocks/text-columns/editor.css 95 B 0 B
build/block-library/blocks/text-columns/style-rtl.css 166 B 0 B
build/block-library/blocks/text-columns/style.css 166 B 0 B
build/block-library/blocks/verse/editor-rtl.css 62 B 0 B
build/block-library/blocks/verse/editor.css 62 B 0 B
build/block-library/blocks/verse/style-rtl.css 87 B 0 B
build/block-library/blocks/verse/style.css 87 B 0 B
build/block-library/blocks/video/editor-rtl.css 504 B 0 B
build/block-library/blocks/video/editor.css 503 B 0 B
build/block-library/blocks/video/style-rtl.css 193 B 0 B
build/block-library/blocks/video/style.css 193 B 0 B
build/block-library/common-rtl.css 1.01 kB 0 B
build/block-library/common.css 1.01 kB 0 B
build/block-library/style-rtl.css 8.8 kB 0 B
build/block-library/style.css 8.8 kB 0 B
build/block-library/theme-rtl.css 748 B 0 B
build/block-library/theme.css 748 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/customize-widgets/style-rtl.css 168 B 0 B
build/customize-widgets/style.css 168 B 0 B
build/edit-widgets/style-rtl.css 3.2 kB 0 B
build/edit-widgets/style.css 3.2 kB 0 B
build/editor/editor-styles-rtl.css 543 B 0 B
build/editor/editor-styles.css 545 B 0 B
build/editor/style-rtl.css 3.89 kB 0 B
build/editor/style.css 3.89 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 637 B 0 B
build/format-library/style.css 639 B 0 B
build/html-entities/index.js 622 B 0 B
build/list-reusable-blocks/style-rtl.css 629 B 0 B
build/list-reusable-blocks/style.css 628 B 0 B
build/nux/style-rtl.css 731 B 0 B
build/nux/style.css 727 B 0 B
build/primitives/index.js 1.42 kB 0 B
build/react-i18n/index.js 1.45 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

@grzim grzim mentioned this pull request Feb 12, 2021
6 tasks
@tellthemachines
Copy link
Contributor

Would be good to add a story for this component so we can better test this PR. All components in this package are supposed to have one, anyway.

@grzim grzim added [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Feb 17, 2021
@ItsJonQ
Copy link

ItsJonQ commented Feb 19, 2021

@grzim This is an interesting idea! I checked out the code and the story.
I may have an idea for how we can enhance this :D. I'll comment with an example soon!

@ItsJonQ ItsJonQ added the Needs Accessibility Feedback Need input from accessibility label Feb 19, 2021
@ItsJonQ
Copy link

ItsJonQ commented Feb 19, 2021

@grzim Haii!! Here's my idea :)

I think this interaction would work very well as a hook.
From what I've seen, the "trigger" to toggle the editable input may vary greatly.
Sometimes its a link, or a heading, or something else.
Also, the editable field may be vary as well (input or textarea).

However, the editing toggling mechanism remains the same - it's just the elements may be different :).

With that in mind, I refactored the main logic from the component into a dedicated hook:

function cancelEvent( event ) {
	event.preventDefault();
	event.stopPropagation();
}

function mergeEvent( handler, otherHandler ) {
	return ( event ) => {
		if ( typeof handler === 'function' ) {
			handler( event );
		}
		if ( typeof otherHandler === 'function' ) {
			otherHandler( event );
		}
	};
}

function useInlineEdit( {
	validate = negate( isUndefined ),
	onChange = noop,
	value: valueProp,
} ) {
	const [ isEdit, setIsEdit ] = useState( false );
	const [ editingValue, setEditingValue ] = useState( valueProp );
	const inputRef = useRef();
	const toggleRef = useRef();

	useEffect( () => {
		if ( isEdit ) {
			inputRef.current.focus();
			inputRef.current.select();
			setEditingValue( valueProp );
		} else {
			toggleRef.current.focus();
		}
	}, [ isEdit, valueProp ] );

	const handleOnCommit = ( event ) => {
		const { value } = event.target;
		cancelEvent( event );
		if ( validate( value ) ) {
			setIsEdit( false );
			onChange( value );
		}
	};

	const handleOnChange = ( event ) => {
		setEditingValue( event.target.value );
	};

	const handleOnKeyDown = ( event ) => {
		if ( 'Enter' === event.key ) {
			handleOnCommit( event );
		}
		if ( 'Escape' === event.key ) {
			cancelEvent( event );
			event.target.blur();
		}
	};

	const handleOnClick = () => setIsEdit( true );

	const getInputProps = ( inputProps ) => ( {
		ref: inputRef,
		onChange: mergeEvent( handleOnChange, inputProps?.onChange ),
		onKeyDown: mergeEvent( handleOnKeyDown, inputProps?.onKeyDown ),
		onBlur: mergeEvent( handleOnCommit, inputProps?.onBlur ),
	} );
	const getToggleProps = ( toggleProps ) => ( {
		ref: toggleRef,
		onClick: mergeEvent( handleOnClick, toggleProps?.toggleProps ),
	} );

	const value = isEdit ? editingValue : valueProp;

	return {
		isEdit,
		getInputProps,
		getToggleProps,
		value,
	};
}

Note: This doesn't have to be the final implementation of the hook. There are probably some enhancements + refactors we can do .

I updated the hook to better handle value when in isEdit mode and not.

This hook doesn't have any UI. You would bind it to UI like this:

function MyCustomEditInPlaceControl( { value: valueProp, onChange = noop } ) {
	const { isEdit, getInputProps, getToggleProps, value } = useInlineEdit( {
		onChange,
		value: valueProp,
	} );

	const customHandleOnClick = noop;

	return (
		<>
			{ isEdit ? (
				<input { ...getInputProps() } value={ value } />
			) : (
				<button
					{ ...getToggleProps( { onClick: customHandleOnClick } ) }
				>
					{ value }
				</button>
			) }
		</>
	);
}

With this setup, we can use whatever elements we need to. We also have the freedom to add whatever callback handlers we need as well (e.g. onKeyDown, etc...)

Let me know what you think :)

Also, cc'ing @diegohaz for thoughts! I've learned a lot about hook-based setups from Reakit. Would love to know your thoughts Diego! I think we could perhaps add some a11y handling in the hook as well.

❤️

@grzim
Copy link
Contributor Author

grzim commented Feb 22, 2021

It looks cool as a hook. To which package should it be added though? It is not a component now

@ItsJonQ
Copy link

ItsJonQ commented Feb 22, 2021

To which package should it be added though?

@grzim That's a great question. I'm not sure 🤔 .
Personally, I'd keep it as part of the /components package, as it's very UI component orientated with a very specific use case.

The general hooks package is /compose. This new hook interaction doesn't feel like it belongs there.

cc'ing @sarayourfriend What do you think :)

@sarayourfriend
Copy link
Contributor

I think we should add it to components and follow the strategy we've been following with the new components in ui.

Move the hook into a hook.js, move the component into a component.js and then export both from index.js.

@ItsJonQ
Copy link

ItsJonQ commented Feb 22, 2021

@sarayourfriend I don't think we'd need an actual component with this one. Just the hook :).

Some maybe something like:

  • /components/inline-edit/index.js
  • /components/inline-edit/hook.js
  • /components/inline-edit/README.md

useInlineEdit marked as ustable

Co-authored-by: sarayourfriend <[email protected]>
Base automatically changed from master to trunk March 1, 2021 15:45
@diegohaz
Copy link
Member

diegohaz commented Mar 5, 2021

Great work @grzim!

I don't think we should encourage this component/hook to be used for toolbar items though (like in your BlockControls example). Referencing my comment on #28614:

This issue has been discussed in a few places already, like in #23375 (comment), where a solution has been proposed. @adamziel started implementing it on #24021. But I think in the end we agreed on fixing this at the design level by using popovers instead of inline editing on toolbars.

The problem is that it's really tricky for keyboard and screen reader users to navigate within the toolbar (which requires arrow key navigation according to the WAI-ARIA recommendations) when there are text fields in it.

If there's a toolbar with a text input element, it's better not to use the role="toolbar" element.

Besides that, this component alone is really hard to get right in terms of accessibility. When using a screen reader, you'll hear button label, button where the button label is a state, not an action. There's no indication that clicking on the button will replace it with an input. I'd say it's not so obvious even for sighted users.

The simplest way to handle this interaction is to have a separate button for the action (for example, with a pencil icon and an explicit or implicit label) and keep the state as a text element. I see no problem in replacing this separate button with an input and temporarily hiding the text element while the input has focus.

@grzim
Copy link
Contributor Author

grzim commented Mar 10, 2021

@diegohaz the current PR consists of a hook only (please see the updated description). Any kind of aria-labels, roles etc. should be used with components and they are not a part of a hook itself (hooks are part of the business logic and should not be tightly coupled with view).
Please let me know if have any concerns about a11y when it comes to the mechanic of the hook.

@grzim grzim changed the title Edit in place control component Edit in place hook Mar 11, 2021
Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

The types and code LGTM but lets wait for @diegohaz to respond about the a11y concerns. I want to make sure we're not introducing any negative patterns by accident 🙂 Great work so far, thank you!

packages/components/src/inline-edit/hook.js Outdated Show resolved Hide resolved
Comment on lines 58 to 61
useEffect( () => {
setEditingValue( propValue );
if ( isInvalid( value ) ) onWrongInput( value );
}, [ propValue ] );
Copy link
Member

Choose a reason for hiding this comment

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

isInvalid and onWrongInput functions are being omitted from the effect dependencies. I understand it would be annoying to include them there since those props could be passed as inline functions and would trigger the effect on every re-render. But I think we should at least add a comment explaining that the effect will get out of sync if the validate logic is changed while value remains the same. I remember @ItsJonQ worked on something similar.

The local value is also omitted from the deps.

Can't we just call onWrongInput on the onChange/onBlur event? It would be much simpler than doing that in an effect.

Copy link

@ItsJonQ ItsJonQ Mar 19, 2021

Choose a reason for hiding this comment

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

I remember @ItsJonQ worked on something similar.

@diegohaz Ah! Are you referring to this usePropRef pattern I came up with?
https://github.com/ItsJonQ/g2/blob/main/packages/utils/src/hooks/use-prop-ref.js

// example
const propRefs = usePropRef({ value, step })

const increment = useCallback(() => {
  const { value, step } = propRefs.current
  onChange(value + step)
}, [onChange, propRefs])

call onWrongInput on the onChange/onBlur event?

I haven't tried it, but that seems like that would be simpler 🤔

Copy link
Contributor Author

@grzim grzim Mar 19, 2021

Choose a reason for hiding this comment

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

@diegohaz As far as I understand what you mean is that we want to make it possible the component will react if a validation (or error handling) function changes. From my perspective, we can do only two things: add those functions as the dependencies and the component will rerender every time, which will lead to poor performance or to skip these dependencies (with a comment as you suggest) as I think it is an edge case.

Solution with usePropRef would not apply here as the component would not react for a function change, please see the sandbox:
https://codesandbox.io/s/suspicious-sanne-74lv9?file=/src/App.js

We can use this pattern though, in order to add references to the hooks dependency array and not to make it rerender every time. This will make a code cleaner but the problem will remain the same.

But all in all, if we validate the value only onCommit (see my recent commit) then the entire problem just vanishes 🦩

packages/components/src/inline-edit/hook.js Outdated Show resolved Hide resolved
packages/components/src/inline-edit/hook.js Outdated Show resolved Hide resolved
@diegohaz
Copy link
Member

@grzim Hmm! I think now the description lacks a reason why we need this hook in the first place. Do you have concrete use cases in mind?

My concern is that this hook encourages a pattern that is really hard to get right in terms of accessibility, and this is a design problem. Are we considering alternatives to this pattern? Like this one:

The simplest way to handle this interaction is to have a separate button for the action (for example, with a pencil icon and an explicit or implicit label) and keep the state as a text element. I see no problem in replacing this separate button with an input and temporarily hiding the text element while the input has focus.

@ItsJonQ
Copy link

ItsJonQ commented Mar 19, 2021

Do you have concrete use cases in mind?

@grzim + @diegohaz Haiii!! Helping to expand on this..

I personally can't identify a specific application interaction at the moment.
A scenario I can think of would be in UI with a lot of fields with limited space.
A table-like UI would be a good example:

Screen Capture on 2021-03-19 at 13-37-16

I've also seen this pattern used in UI like contact/user/profiles.
I suppose the use case would be:

To provide a way to quickly edit a single field in UI with many fields within limited space.
These fields may or may not be accompanied by an "Edit" button.

Implementation aside... Do you think this can work from an a11y perspective?


I see no problem in replacing this separate button with an input and temporarily hiding the text element while the input has focus.

I'm having a bit of trouble imagining this one 😊 . Would you be able to mock it up super fast in a CodeSandbox?

Thank you 🙏 !

@diegohaz
Copy link
Member

A table-like UI would be a good example:

Screen Capture on 2021-03-19 at 13-37-16

That's a data grid, where role="gridcell" elements can contain interactive and editable content. Some screen readers like NVDA will even announce the cell as "editable" when you focus on it. That's how a spreadsheet app can be structured, for example.

Here's an example: https://codesandbox.io/s/reakit-data-grid-m95te

The inputs are already there in the markup. They aren't buttons that turn into inputs.

Implementation aside... Do you think this can work from an a11y perspective?

As I said, this component, the way it's designed here, is really hard to get right. If we don't have enough space to show an "Edit" button, let's just render an input with a default value and give it appropriate styles on focus or hover. I still think that the lack of indication that it's an editable element is problematic even for sighted users, but at least keyboard and screen reader users wouldn't be affected.

The problem is that this component has both a state (the value) and an action (edit the value). Unlike <input>, a simple <button> or a role="button" element can only contain a label, and it's usually (or preferrably) a call to action. To make this button work here we would have to play with aria descriptions (which are much less important than labels) or visually hidden text (like Edit "Title" value, where only Title is visible) to properly communicate all the information. I don't think it's a good solution.

"Buttons" that represent both state and action have different semantics (like <select>).

I see no problem in replacing this separate button with an input and temporarily hiding the text element while the input has focus.

I'm having a bit of trouble imagining this one 😊 . Would you be able to mock it up super fast in a CodeSandbox?

Sure! Here's a CodeSandbox with the two alternatives I had in mind: https://codesandbox.io/s/edit-in-place-bvn2o

The first one is just an input, which is the most simple solution I can think of. The second one is the one you asked about, but I ended up using a read-only input that turns into an editable input instead of replacing the element. The implementation could be improved, but I think the input and the button together communicate more clearly the state and the action.

I guess the challenge there is controlling the width of the input.

@grzim
Copy link
Contributor Author

grzim commented Mar 23, 2021

This thread took a long way beginning in #25343 where designs included an edit-in-place component. As it was not needed there was an idea to leave this component anyway if there is a solid reason to do so. The next iteration was to change it into a hook to make it more flexible.
All in all, if there is no solid reason to add it to the codebase, and with regard to the YAGNI rule, I would not add it, as we don't need dead code in our repo. Also, @diegohaz I totally agree that from the semantic point of view the HTML element that shows state and is also a source of action has its own name and it is <select> ;) So if there is no idea, how the hook can be used I think this issue can be closed.

@grzim grzim closed this Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Accessibility Feedback Need input from accessibility [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants