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

Jump to bottom

Small fix: add keyboard esc support for color picker #528

Closed
wants to merge 1 commit into from
Closed

Small fix: add keyboard esc support for color picker #528

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions assets/src/edit-story/components/colorPicker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { __ } from '@wordpress/i18n';
import { PatternPropType } from '../../types';
import useFocusOut from '../../utils/useFocusOut';
import { Close } from '../button';
import { useKeyDownEffect } from '../keyboard';
import CurrentColorPicker from './currentColorPicker';
import useColor from './useColor';

Expand Down Expand Up @@ -92,6 +93,9 @@ function ColorPicker({ color, hasGradient, hasOpacity, onChange, onClose }) {
const previousFocus = useRef(document.activeElement);

useFocusOut(containerRef, onClose);
useKeyDownEffect(containerRef, { key: 'esc', editable: true }, onClose, [
onClose,
]);
Comment on lines +96 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

esc support is coming in #497, FYI.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the lack of { editable: true } in that line in 497 is actually intentional. If you're editing either hex or opacity, you can press esc to just exit editing this field only, but not close the entire color picker. Pressing esc again will then close the color picker.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pressing ESC again will then close the color picker.

@barklund That doesn't seem to always work, e.g. when changing the page background color. And when editing text color, it not only closes the the color picker, but actually unselects the text element.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, those are good points. We will need to check the event flow better for this to work consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So, I can close this then, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so, yes.

@barklund can you make sure there's an issue for the things I mentioned?


useLayoutEffect(() => {
closeRef.current.focus();
Expand Down