Add VPopover component #397

sarayourfriend · 2021-11-05T14:22:53Z

Fixes

Description

Adds a new VPopover component which composes a "trigger" and popover content to create a single, acessible, popover unit.

VPopoverContent can be reused generically if, for any reason (there shouldn't really be any for the most part) a popover is needed with a different trigger mechanism. It implements 95% of the accessibility functionality (only aria-haspopup and aria-expanded are handled by the parent VPopover component).

Most of the logic for this component is siloed into individual responsibility composables. Most of this logic is copied from Reakit's Dialog and Popover implementations.

VPopoverContent emits all normal div events.

VPopover emits open and close events.

Testing Instructions

Run npm run storybook and visit http://localhost:6006/?path=/docs/components-vpopover--default. Play around with the popover and the various props to make sure it behaves as expected according to the prop descriptions. Unit tests cover almost everything except one edge case that isn't possible to test in JSDom specific to text selection.

To test this particular behavior, open the popover and make sure hideOnClickOutside is either unset or set to true. Then click and drag to select text inside the popover but be sure to finish the drag outside the popover and then release the mouse click. This should not close the popover.

This is important because the click event is actually happening outside the popover, but in the useHideOnClickOutside composable we conditionally hide the popover only when the mousedown event target matches the click target or both are outside the popover. This ensures that the popover doesn't accidentally close if someone is selecting the text inside of it.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend · 2021-11-05T14:24:39Z

src/composables/use-focus-on-blur.js

+  const blurredRef = ref(0)
+
+  /**
+   * @param {number} n
+   */
+  const scheduleFocus = () => (blurredRef.value += 1)
+
+  return [blurredRef, scheduleFocus]


I think this could be turned into a customRef, but then it would need to use setting to update the value instead of just calling a function that does it for you.

Ehhhh, I think I prefer this slimmer more obvious approach.

zackkrida

This is such comprehensive work, and the new composables created here will be a great resource for other future components. I wish I had more feedback to offer!

zackkrida · 2021-11-10T20:25:56Z

src/components/.eslintrc.js

-      { case: 'pascalCase', ignore: ['.eslintrc.js', '.*\\.stories\\.js'] },
+      // Allow things like `Component.stories.js` and `Component.types.js`
+      {
+        case: 'pascalCase',
+        ignore: ['.eslintrc.js', '.*\\..*\\.js'],
+      },


zackkrida · 2021-11-10T20:26:32Z

src/utils/warn.js

@@ -0,0 +1,2 @@
+export const warn =


Great addition to the codebase

zackkrida · 2021-11-10T20:28:51Z

src/components/VPopover/VPopoverContent.vue

+  /**
+   * This is the only documented emitted event but in reality we pass through `$listeners`
+   * to the underlying element so anything and everything is emitted. `@keydown` is the
+   * only one this component overrides and controls (but ultimately still emits).
+   */


zackkrida · 2021-11-10T20:31:18Z

src/components/VPopover/VPopover.vue

+      :aria-labelledby="labelledBy"
+    >
+      <!-- @slot The content of the popover -->
+      <slot name="content" />


Since this doesn't appear to have a ref or any props passed to the slot, should we make this the default slot to make the component api a little simpler? I think there's also something compelling about having the two explicit slots in the current implementation, so up to you.

Yeah! We can do that.

zackkrida · 2021-11-11T16:25:13Z

The improvements are really nice, especially the test with two popovers.

sarayourfriend · 2021-11-15T19:26:30Z

@obulat Do you think you will have time to review this PR this week?

obulat

I love the thoroughness of the implementation! It all works great, and is accessible.

src/components/VPopover/VPopoverContent.vue

obulat · 2021-11-16T03:59:45Z

src/components/VPopover/VPopoverContent.vue

+    })
+
+    /**
+     * @param {Event} event


Does the Event type resolve to something useful for you? When I try going to its source in VS Code, it shows me docs from React, and WebStorm goes to sentry/types ... I've ran into such typing problems before, and couldn't find a good solution.

Hmm, it shouldn't reference either, it should reference the global Event type from TypeScript's built in dom.d.ts: https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts#L5034

When I hover over it it shows the description from the global dom types but if I "go to definition" on it in VSCode it does show React and the global dom types. I think the React types get detected because of interface merging and because React declares global types. Luckily they don't modify the Event type in this way. I have no idea what WebStorm pulls types from sentry without an explicit reference to it.

src/composables/use-hide-on-click-outside.js

obulat · 2021-11-16T04:05:08Z

 WARN  in ./.nuxt/components/index.js                                                                                                            friendly-errors 06:43:56

"export 'default' (reexported as 'VPopoverContentTypes') was not found in '../../src/components/VPopover/VPopoverContent.types.js'

Do you know why I get this warning when I run npm run dev or npm run storybook?

obulat · 2021-11-16T04:06:17Z

src/composables/use-focus-on-blur.js

+/**
+ * @typedef Props
+ * @property {import('./types').Ref<HTMLElement>} popoverRef
+ * @property {import('./types').ToRefs<import('../components/VPopover/VPopover.types').Props>} popoverPropsRefs


WebStorm shows a warning that VPopover.types is not found.

I don't think WebStorm is using TypeScript to interpret the JSDoc comments.

src/composables/use-focus-on-blur.js

src/composables/use-focus-on-hide.js

sarayourfriend · 2021-11-16T14:34:38Z

Do you know why I get this warning when I run npm run dev or npm run storybook?

Yeah, it's because of Nuxt's global component resolution trying to find a component in every single file in the VPopover folder 😞 We could remove it from the global component resolution configuration in nuxt.config and I think it would remove that warning.

Co-Authored-By: obulat

* main: Create the `InputField` and `SearchBar` components (#380) Add VPopover component (#397) Search and media store refactoring (#398) Node 16 and NPM 8 (#413)

sarayourfriend commented Nov 5, 2021

View reviewed changes

sarayourfriend force-pushed the add/v-popover branch from 0703317 to 9d2cc24 Compare November 9, 2021 14:04

zackkrida requested review from obulat and zackkrida November 9, 2021 15:22

zackkrida approved these changes Nov 10, 2021

View reviewed changes

sarayourfriend changed the title ~~WIP Add VPopover~~ Add VPopover component Nov 11, 2021

sarayourfriend self-assigned this Nov 11, 2021

sarayourfriend added 🌟 goal: addition Addition of new feature 🕹 aspect: interface Concerns end-users' experience with the software labels Nov 11, 2021

sarayourfriend marked this pull request as ready for review November 11, 2021 15:32

sarayourfriend requested a review from a team as a code owner November 11, 2021 15:32

sarayourfriend added 10 commits November 11, 2021 07:33

Add Popover component

399ffd0

Simplify some things

f8c7efa

Fix popper not attaching to popover

735eb57

Fix button tests

baf5f9b

Simplify focus management and only allow a single disclosure

88d4338

Remove stray console log

bc31ccb

Fix initial focus issue

87df2a8

Fix focus management

c3f779a

Document props and events

c461746

Use default slot for popover content

2ecdf6d

sarayourfriend force-pushed the add/v-popover branch from 596a214 to 2ecdf6d Compare November 11, 2021 15:38

obulat approved these changes Nov 16, 2021

View reviewed changes

obulat reviewed Nov 16, 2021

View reviewed changes

src/composables/use-focus-on-blur.js Outdated Show resolved Hide resolved

obulat reviewed Nov 16, 2021

View reviewed changes

src/composables/use-focus-on-hide.js Outdated Show resolved Hide resolved

obulat reviewed Nov 16, 2021

View reviewed changes

src/composables/use-focus-on-hide.js Show resolved Hide resolved

Various clean ups

0ba36ba

Co-Authored-By: obulat

sarayourfriend merged commit 3922733 into main Nov 16, 2021

sarayourfriend deleted the add/v-popover branch November 16, 2021 15:18

rbadillap added a commit that referenced this pull request Nov 17, 2021

Merge branch 'main' into ci/pipelines

53c0d26

* main: Create the `InputField` and `SearchBar` components (#380) Add VPopover component (#397) Search and media store refactoring (#398) Node 16 and NPM 8 (#413)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add VPopover component #397

Add VPopover component #397

sarayourfriend commented Nov 5, 2021 •

edited

Loading

sarayourfriend Nov 5, 2021 •

edited

Loading

zackkrida left a comment

zackkrida Nov 10, 2021

zackkrida Nov 10, 2021

zackkrida Nov 10, 2021

zackkrida Nov 10, 2021

sarayourfriend Nov 11, 2021

zackkrida commented Nov 11, 2021

sarayourfriend commented Nov 15, 2021

obulat left a comment

obulat Nov 16, 2021

sarayourfriend Nov 16, 2021

obulat commented Nov 16, 2021

obulat Nov 16, 2021

sarayourfriend Nov 16, 2021

sarayourfriend commented Nov 16, 2021

Add VPopover component #397

Add VPopover component #397

Conversation

sarayourfriend commented Nov 5, 2021 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackkrida commented Nov 11, 2021

sarayourfriend commented Nov 15, 2021

obulat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obulat commented Nov 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarayourfriend commented Nov 16, 2021

sarayourfriend commented Nov 5, 2021 •

edited

Loading

sarayourfriend Nov 5, 2021 •

edited

Loading