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

Update some React 18 related types #45279

Merged
merged 9 commits into from Oct 26, 2022
4 changes: 1 addition & 3 deletions packages/components/src/form-file-upload/README.md
Expand Up @@ -81,9 +81,7 @@ This can be useful when you want to force a `change` event to fire when the user

### render

Optional callback function used to render the UI. If passed the component does not render any UI and calls this function to render it.

This function receives an object with the property `openFileDialog`. The property is a function that when called opens the browser window to upload files.
Optional callback function used to render the UI. If passed, the component does not render the default UI (a button) and calls this function to render it. The function receives an object with property `openFileDialog`, a function that, when called, opens the browser native file upload modal window.

- Type: `Function`
- Required: No
1 change: 1 addition & 0 deletions packages/components/src/form-file-upload/index.tsx
Expand Up @@ -47,6 +47,7 @@ export function FormFileUpload( {
{ children }
</Button>
);

return (
<div className="components-form-file-upload">
{ ui }
Expand Down
9 changes: 5 additions & 4 deletions packages/components/src/form-file-upload/types.ts
Expand Up @@ -55,9 +55,10 @@ export type FormFileUploadProps = {
/**
* Optional callback function used to render the UI.
*
* If passed, the component does not render any UI and calls this function to render it.
* This function receives an object with the property `openFileDialog`.
* The property is a function that when called opens the browser window to upload files.
* If passed, the component does not render the default UI (a button) and
* calls this function to render it. The function receives an object with
* property `openFileDialog`, a function that, when called, opens the browser
* native file upload modal window.
*/
render?: ( arg: { openFileDialog: () => void } ) => void;
render?: ( arg: { openFileDialog: () => void } ) => ReactNode;
};
28 changes: 0 additions & 28 deletions packages/components/src/isolated-event-container/index.js

This file was deleted.

32 changes: 32 additions & 0 deletions packages/components/src/isolated-event-container/index.tsx
@@ -0,0 +1,32 @@
/**
* External dependencies
*/
import type { ComponentPropsWithoutRef, MouseEvent } from 'react';

/**
* WordPress dependencies
*/
import { forwardRef } from '@wordpress/element';
import deprecated from '@wordpress/deprecated';

function stopPropagation( event: MouseEvent ) {
event.stopPropagation();
}

type DivProps = ComponentPropsWithoutRef< 'div' >;

const IsolatedEventContainer = forwardRef< HTMLDivElement, DivProps >(
( props, ref ) => {
deprecated( 'wp.components.IsolatedEventContainer', {
since: '5.7',
} );

// Disable reason: this stops certain events from propagating outside of the component.
// - onMouseDown is disabled as this can cause interactions with other DOM elements.
/* eslint-disable jsx-a11y/no-static-element-interactions */
return <div { ...props } ref={ ref } onMouseDown={ stopPropagation } />;
/* eslint-enable jsx-a11y/no-static-element-interactions */
}
);

export default IsolatedEventContainer;
4 changes: 2 additions & 2 deletions packages/components/src/popover/index.tsx
Expand Up @@ -252,7 +252,7 @@ const UnforwardedPopover = (
} );
}

const arrowRef = useRef( null );
const arrowRef = useRef< HTMLElement | null >( null );

const [ fallbackReferenceElement, setFallbackReferenceElement ] =
useState< HTMLSpanElement | null >( null );
Expand Down Expand Up @@ -385,7 +385,7 @@ const UnforwardedPopover = (
} );

const arrowCallbackRef = useCallback(
( node ) => {
( node: HTMLElement | null ) => {
arrowRef.current = node;
update();
},
Expand Down
15 changes: 13 additions & 2 deletions packages/components/src/toggle-control/index.tsx
Expand Up @@ -64,8 +64,19 @@ export function ToggleControl( {

let describedBy, helpLabel;
if ( help ) {
describedBy = id + '__help';
helpLabel = typeof help === 'function' ? help( checked ) : help;
if ( typeof help === 'function' ) {
// `help` as a function works only for controlled components where
// `checked` is passed down from parent component. Uncontrolled
// component can show only a static help label.
if ( checked !== undefined ) {
helpLabel = help( checked );
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 it would make sense to allow help to be called with an undefined, as it currently is on trunk. The docs don't mention anything about help working only for controlled components

Copy link
Member Author

Choose a reason for hiding this comment

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

could you add a CHANGELOG entry too?

Added an entry about the ToggleControl's help prop type. Other changes in this PR are not publicly visible.

Copy link
Member Author

@jsnajdr jsnajdr Oct 26, 2022

Choose a reason for hiding this comment

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

I wonder if it would make sense to allow help to be called with an undefined

That's an incorrect behavior though. If the checked prop is not specified, then the input in uncontrolled, i.e., it stores the checked state internally and only fires the onChange events. The user can check the toggle, it will fire the onChange event, will be checked visually, but the checked prop variable will still be undefined. The wrong help will be shown.

After a closer look, I see that ToggleControl doesn't really support uncontrolled mode correctly. A checked FormToggle component won't have the is-checked CSS class, because the checked prop is never true. Also the component doesn't have a defaultChecked prop for setting initial uncontrolled value.

The checked value shouldn't be allowed to be undefined. It only creates an illusion of supporting uncontrolled mode, but the support is not really there. And all usages in the Gutenberg codebase seem to be controlled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you again for looking into this! Your reasoning makes sense to me.

After a closer look, I see that ToggleControl doesn't really support uncontrolled mode correctly.

That seems to be an issue most of the older control components in the library, thank you for flagging

}
} else {
helpLabel = help;
}
if ( helpLabel ) {
describedBy = id + '__help';
}
}

return (
Expand Down
6 changes: 2 additions & 4 deletions packages/components/src/toggle-control/types.ts
Expand Up @@ -13,10 +13,8 @@ export type ToggleControlProps = Pick<
FormToggleProps,
'checked' | 'disabled'
> &
Pick<
BaseControlProps,
'__nextHasNoMarginBottom' | 'help' | 'className'
> & {
Pick< BaseControlProps, '__nextHasNoMarginBottom' | 'className' > & {
help?: ReactNode | ( ( checked: boolean ) => ReactNode );
/**
* The label for the toggle.
*/
Expand Down
4 changes: 2 additions & 2 deletions packages/compose/src/hooks/use-dialog/index.ts
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import type { KeyboardEvent, RefCallback, SyntheticEvent } from 'react';
import type { RefCallback, SyntheticEvent } from 'react';

/**
* WordPress dependencies
Expand Down Expand Up @@ -64,7 +64,7 @@ function useDialog( options: DialogOptions ): useDialogReturn {
currentOptions.current.onClose();
}
} );
const closeOnEscapeRef = useCallback( ( node ) => {
const closeOnEscapeRef = useCallback( ( node: HTMLElement ) => {
if ( ! node ) {
return;
}
Expand Down
80 changes: 44 additions & 36 deletions packages/compose/src/hooks/use-focus-outside/index.js
Expand Up @@ -125,54 +125,62 @@ export default function useFocusOutside( onFocusOutside ) {
* intends to normalize this as treating click on buttons as focus.
*
* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*
* @param {SyntheticEvent} event Event for mousedown or mouseup.
*/
const normalizeButtonFocus = useCallback( ( event ) => {
const { type, target } = event;
const isInteractionEnd = [ 'mouseup', 'touchend' ].includes( type );

if ( isInteractionEnd ) {
preventBlurCheck.current = false;
} else if ( isFocusNormalizedButton( target ) ) {
preventBlurCheck.current = true;
}
}, [] );
const normalizeButtonFocus = useCallback(
/**
* @param {SyntheticEvent} event Event for mousedown or mouseup.
*/
( event ) => {
const { type, target } = event;
const isInteractionEnd = [ 'mouseup', 'touchend' ].includes( type );

if ( isInteractionEnd ) {
preventBlurCheck.current = false;
} else if ( isFocusNormalizedButton( target ) ) {
preventBlurCheck.current = true;
}
},
[]
);

/**
* A callback triggered when a blur event occurs on the element the handler
* is bound to.
*
* Calls the `onFocusOutside` callback in an immediate timeout if focus has
* move outside the bound element and is still within the document.
*
* @param {SyntheticEvent} event Blur event.
*/
const queueBlurCheck = useCallback( ( event ) => {
// React does not allow using an event reference asynchronously
// due to recycling behavior, except when explicitly persisted.
event.persist();

// Skip blur check if clicking button. See `normalizeButtonFocus`.
if ( preventBlurCheck.current ) {
return;
}

blurCheckTimeoutId.current = setTimeout( () => {
// If document is not focused then focus should remain
// inside the wrapped component and therefore we cancel
// this blur event thereby leaving focus in place.
// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus.
if ( ! document.hasFocus() ) {
event.preventDefault();
const queueBlurCheck = useCallback(
/**
* @param {SyntheticEvent} event Blur event.
*/
( event ) => {
// React does not allow using an event reference asynchronously
// due to recycling behavior, except when explicitly persisted.
event.persist();

// Skip blur check if clicking button. See `normalizeButtonFocus`.
if ( preventBlurCheck.current ) {
return;
}

if ( 'function' === typeof currentOnFocusOutside.current ) {
currentOnFocusOutside.current( event );
}
}, 0 );
}, [] );
blurCheckTimeoutId.current = setTimeout( () => {
// If document is not focused then focus should remain
// inside the wrapped component and therefore we cancel
// this blur event thereby leaving focus in place.
// https://developer.mozilla.org/en-US/docs/Web/API/Document/hasFocus.
if ( ! document.hasFocus() ) {
event.preventDefault();
return;
}

if ( 'function' === typeof currentOnFocusOutside.current ) {
currentOnFocusOutside.current( event );
}
}, 0 );
},
[]
);

return {
onFocus: cancelBlurCheck,
Expand Down