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 Copy to Clipboard recipe to show/hide on hover/focus events #11802

Merged
merged 18 commits into from Apr 1, 2024

Conversation

aaronccasanova
Copy link
Member

@aaronccasanova aaronccasanova commented Mar 28, 2024

WHAT is this pull request doing?

This PR updates the Copy to Clipboard recipe to show/hide the copy button on hover/focus events. This was accomplished by adding a few more hooks to the system:

  • useHover - Tracks mouseenter/mouseleave events on a target ref
  • useFocus - Tracks focus/blur events on a target ref
  • useFocusIn - Tracks focusin/focusout events on a target ref
  • useMediaQuery - Tracks change events on a target matchMedia(query). Additionally, supports queryAliases to streamline the application of common media queries e.g.
    const isMouseDevice = useMediaQuery('mouse')

Example: Show the copy button on mouseenter of the target ref:

Screen.Recording.2024-03-28.at.11.43.12.AM.mov

Example: Always show the copy button on non-mouse devices

Screen.Recording.2024-03-28.at.11.44.31.AM.mov

Example: Show the copy button on focusin of the target ref

Screen.Recording.2024-03-28.at.2.20.51.PM.mov

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@aaronccasanova aaronccasanova added the 🤖Skip Changelog Causes CI to ignore changelog update check. label Mar 28, 2024
@aaronccasanova aaronccasanova changed the title Update Copy to Clipboard recipe to show/hide on mouseHover/focus Update Copy to Clipboard recipe to show/hide on hover/focus events Mar 28, 2024
@aaronccasanova aaronccasanova self-assigned this Mar 29, 2024
@aaronccasanova aaronccasanova added the #gsd:38421 Polaris Common Action Patterns label Mar 29, 2024
@aaronccasanova aaronccasanova marked this pull request as ready for review March 29, 2024 00:10
Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

🎉 👏 🚀

Comment on lines +32 to +45
const handleFocusIn = useCallback(() => {
if (deferredFocusOut.current) {
clearTimeout(deferredFocusOut.current);
deferredFocusOut.current = null;
}
setIsFocusedIn(true);
}, []);

const handleFocusOut = useCallback(() => {
// Push focusout state update to the end of the event queue to
// allow subsequent focusin events to persist the isFocusedIn state
deferredFocusOut.current = setTimeout(() => {
setIsFocusedIn(false);
}, 0);
Copy link
Member Author

Choose a reason for hiding this comment

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

Prevents the internal state from toggling off and on when moving focus between child elements

Copy link
Member

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

The code and examples look great!

Curious to know more about the feedback that was given on why we made these UX changes? The show/hide of the copy button after hover and focus seem like a two-step process rather than one.

@aaronccasanova did you consider composing a useMediaQuery hook with the useHover instead of creating a useMouseHover? Still the same number of hooks, but maybe more flexible in the long-term 😄

I could see something like:

// predefined set of media or custom string
const isMouseDevice = useMediaQuery("mouse");
const isHover = useHover(ref);
const isMouseHover = isMouseDevice ? isHover : fallback;

@aaronccasanova
Copy link
Member Author

Curious to know more about the feedback that was given on why we made these UX changes? The show/hide of the copy button after hover and focus seem like a two-step process rather than one.

The UX changes were made based on feedback from our Vault review. The suggestion was to not have the copy icon visible at all times, but only when a user hovers over the text. The rationale behind this is that if a user intends to copy a piece of text, their natural instinct would be to bring their cursor to the text. When they do this, the copy icon will be revealed, leading them to the action of copying. I do agree it has the effect of being a two-step process but should be the same number of operations as the original recipe.

@aaronccasanova did you consider composing a useMediaQuery hook with the useHover instead of creating a useMouseHover?

Yes, early explorations were implemented with a useMediaQuery hook (albeit still within the useMouseHover). I ended up removing it to avoid confusion with our existing useMediaQuery hook and reduce overall complexity. That said, I like the idea of moving more logic to the recipe and happy to reintroduce it.

Copy link
Member

@sam-b-rose sam-b-rose left a comment

Choose a reason for hiding this comment

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

💯 ✅

const isFocusedIn = useFocusIn(ref);
const isHovered = useHover(ref);
const isMouseDevice = useMediaQuery('mouse');
const isMouseHovered = isMouseDevice ? isHovered : true;
Copy link
Member

Choose a reason for hiding this comment

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

Very nice! This pattern feels much more composable to me. Thanks for updating this so fast ⚡

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! I am also much happier with this composition 🎉

Copy link
Contributor

@laurkim laurkim left a comment

Choose a reason for hiding this comment

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

Nice! 💯
Just a small suggestion to adjust the vertical alignment of the button in the story but not blocking

Comment on lines +879 to +884
<Button
variant="tertiary"
accessibilityLabel="Copy email address"
onClick={copy}
icon={status === 'copied' ? CheckIcon : ClipboardIcon}
/>
Copy link
Contributor

@laurkim laurkim Apr 1, 2024

Choose a reason for hiding this comment

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

Suggested change
<Button
variant="tertiary"
accessibilityLabel="Copy email address"
onClick={copy}
icon={status === 'copied' ? CheckIcon : ClipboardIcon}
/>
<InlineStack align="center">
<Button
variant="tertiary"
accessibilityLabel="Copy email address"
onClick={copy}
icon={status === 'copied' ? CheckIcon : ClipboardIcon}
/>
</InlineStack>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for flagging the alignment issue! Instead of introducing an element, I added blockAlign="center" to the parent InlineStack and activatorWrapper="div" to the Tooltip:

Before:
Screenshot 2024-04-01 at 11 39 59 AM

After:
Screenshot 2024-04-01 at 11 40 07 AM

Before:
Screenshot 2024-04-01 at 11 40 49 AM

After:
Screenshot 2024-04-01 at 11 41 35 AM
Screenshot 2024-04-01 at 11 41 47 AM

@aaronccasanova aaronccasanova removed the 🤖Skip Changelog Causes CI to ignore changelog update check. label Apr 1, 2024
@aaronccasanova aaronccasanova merged commit 3d93f8d into main Apr 1, 2024
17 checks passed
@aaronccasanova aaronccasanova deleted the update-copy-to-clipboard branch April 1, 2024 21:26
sophschneider pushed a commit that referenced this pull request Apr 2, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @shopify/polaris@12.25.0

### Minor Changes

- [#11802](#11802)
[`3d93f8daf`](3d93f8d)
Thanks [@aaronccasanova](https://github.com/aaronccasanova)! - Added
`useHover`, `useFocus`, `useFocusIn`, and `useMediaQuery` hooks for
building Copy to Clipboard actions


- [#11801](#11801)
[`f6308995e`](f630899)
Thanks [@sophschneider](https://github.com/sophschneider)! - Added white
alpha ramp and more dark experimental tokens

### Patch Changes

- [#11617](#11617)
[`2ff9427b3`](2ff9427)
Thanks [@jesstelford](https://github.com/jesstelford)! - [IndexTable]
Unify sticky table header rendering with regular heading for consistency

- Updated dependencies
\[[`f6308995e`](f630899)]:
    -   @shopify/polaris-tokens@8.10.0

## @shopify/polaris-tokens@8.10.0

### Minor Changes

- [#11801](#11801)
[`f6308995e`](f630899)
Thanks [@sophschneider](https://github.com/sophschneider)! - Added white
alpha ramp and more dark experimental tokens

## @shopify/polaris-migrator@0.28.5

### Patch Changes

- Updated dependencies
\[[`f6308995e`](f630899)]:
    -   @shopify/polaris-tokens@8.10.0
    -   @shopify/stylelint-polaris@15.5.1

## @shopify/stylelint-polaris@15.5.1

### Patch Changes

- Updated dependencies
\[[`f6308995e`](f630899)]:
    -   @shopify/polaris-tokens@8.10.0

## polaris-for-vscode@0.9.8

### Patch Changes

- Updated dependencies
\[[`f6308995e`](f630899)]:
    -   @shopify/polaris-tokens@8.10.0

## polaris.shopify.com@0.67.1

### Patch Changes

- Updated dependencies
\[[`3d93f8daf`](3d93f8d),
[`2ff9427b3`](2ff9427),
[`f6308995e`](f630899)]:
    -   @shopify/polaris@12.25.0
    -   @shopify/polaris-tokens@8.10.0

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AnnaCheba pushed a commit to AnnaCheba/polaris that referenced this pull request Apr 22, 2024
…hopify#11802)

### WHAT is this pull request doing?

This PR updates the Copy to Clipboard recipe to show/hide the copy
button on hover/focus events. This was accomplished by adding a few more
hooks to the system:
  - `useHover` - Tracks `mouseenter`/`mouseleave` events on a target ref
  - `useFocus` - Tracks `focus`/`blur` events on a target ref
  - `useFocusIn` - Tracks `focusin`/`focusout` events on a target ref
- `useMediaQuery` - Tracks `change` events on a target
`matchMedia(query)`. Additionally, supports `queryAliases` to streamline
the application of [common media
queries](https://github.com/argyleink/open-props/blob/09e70c03c0a2533d06ec823f47490f018eb27f23/src/props.media.css#L21-L24)
e.g.
    `const isMouseDevice = useMediaQuery('mouse')`

Example: Show the copy button on `mouseenter` of the target ref:


https://github.com/Shopify/polaris/assets/32409546/b3799bdf-e915-4761-a68c-f2724cccd9f1

Example: Always show the copy button on non-mouse devices


https://github.com/Shopify/polaris/assets/32409546/221d7c76-9a4a-49e2-8be3-8f97d1d7a3b2

Example: Show the copy button on `focusin` of the target ref


https://github.com/Shopify/polaris/assets/32409546/49c078f0-158a-40b3-8494-c15d94f3d0ea

---------

Co-authored-by: Lo Kim <lo.kim@shopify.com>
Co-authored-by: Sam Rose <11774595+sam-b-rose@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
#gsd:38421 Polaris Common Action Patterns
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants