Add hover cards #1067

matthew-white · 2024-11-26T20:06:38Z

What has been done to verify that this works as intended?

I tried things out locally. I've deployed this branch to the QA server, so you can check out the hover cards there.

I've also updated existing tests and written new tests.

Why is this the best possible solution? Were any other approaches considered?

There are a few moving parts:

The hoverCard object (accessible from container) manages the reactive state of the hover card. It's similar in that way to the alert object, which manages the reactive state of the alert. (The Alert component is what actually uses that state to render the alert.)
The useHoverCard() composable sets up event listeners on mouseenter and mouseleave that will modify the hoverCard object. It's also responsible for waiting the 350ms to show the hover card.
Whenever the hoverCard object is modified, the HoverCards component will react to it. HoverCards sends one or more requests for the requestData needed to render the hover card. Once the responses are received, it renders the correct hover card in a popover.
The requestData resources that HoverCards uses to send requests are created in useHoverCardResources(). The resources transform the response data. The resources don't need to be reactive, because once a hover card is shown, it won't change. The useHoverCardResources() composable makes it easier to test various hover card components, since it can be passed to testRequestData().
The actual content of the hover card varies by resource. There is a hover card component for each resource: HoverCardForm, HoverCardSubmission, HoverCardDataset, HoverCardEntity. They all use the HoverCard component, which lays out the structure and styles of the hover card.
What's calling useHoverCard() are new *Link components. There is one for each resource: FormLink, SubmissionLink, DatasetLink, EntityLink. These links are the elements that trigger the hover card to be shown.

I think all these pieces are necessary. I also like that the hoverCard object has parallels to the alert object, as well as modalData().

One goal I had was to keep useHoverCard() fairly flexible and not tightly couple it to the *Link components. I think we're going to use hover cards in other ways in the future (e.g., I think with infonav buttons: getodk/central#827).

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This PR updates a lot existing links to use the new *Link components. I think regression testing will be useful as a check that each of those links is still working as expected. In many cases, there's also good test coverage of the links.

The thing that I feel the most unsure about is whether the hover card is always hidden when it should be. I think so, but that would be a good area for the QA team to explore. (I've noted that in a comment on the release criteria.)

Once we add infonav buttons, we will probably need to hide hover cards in more cases: see getodk/central#800. However, I think we can look into that in the future.

Before submitting this PR, please make sure you have:

run npm run test and npm run lint and confirmed all checks still pass OR confirm CircleCI build passes
verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

matthew-white · 2024-12-09T07:39:21Z

I want to write a few more tests, especially of HoverCardEntity. However, I think there's more than enough to get started on code review. I'll go ahead and mark this PR as ready for review.

matthew-white

Explanatory comments to help with async review

matthew-white · 2024-12-09T06:33:41Z

.eslintrc.js

        'LIST_RENDERING',
-        'CONDITIONALS',
+        ['CONDITIONALS', 'DEFINITION'],


This is because I want to be able to specify v-if before :is. I feel like v-if should generally be the very first thing.

matthew-white · 2024-12-09T06:34:36Z

src/assets/scss/app.scss

@@ -75,7 +74,7 @@ dl:not(.dl-horizontal) {
 // down to the fact that .dl-horizontal uses a CSS grid, while other <dl>
 // elements use flexbox.
 .dl-horizontal {
-  $padding-between: 10px;
+  $padding-between: 12px;


This will have a minor effect on EntityUploadHeaderErrors, but I think that's OK.

matthew-white · 2024-12-09T06:41:32Z

src/components/dataset/link.vue

+const to = computed(() => datasetPath(props.projectId, props.name));
+
+const link = ref(null);
+useHoverCard(computed(() => link.value?.$el), 'dataset', () => props);


We need to pass the HTMLElement of the <router-link> to useHoverCard(). It looks like that's accessible via $el, though I don't see that documented in the Vue Router docs. Apparently $el is generally accessible from components that use the Options API. Components that use the Composition API can opt into providing access to the root element by using defineExpose().

matthew-white · 2024-12-09T06:43:05Z

src/components/dataset/summary/row.vue

@@ -14,9 +14,8 @@ except according to the terms contained in the LICENSE file.
    <div class="row">
      <div class="col-xs-6 dataset-name-wrap">
        <div class="dataset-name text-overflow-ellipsis" v-tooltip.text>
-          <router-link v-if="!dataset.isNew" :to="datasetPath(projectId, dataset.name)" v-tooltip.text>


The parent element specifies v-tooltip.text, so I don't think this one needs to.

matthew-white · 2024-12-09T06:45:42Z

src/components/hover-card.vue

+$border: 1px solid #fff;
+$border-radius: 12px;
+
+.popover:has(.hover-card) { border-radius: $border-radius; }


I had to fight to make the border radius visible on the hover card and spent a bit of time trying to get that to work. Adding border-radius to .popover seemed to be needed.

matthew-white · 2024-12-09T07:03:55Z

src/components/popover.vue

@@ -33,7 +33,7 @@ implementation of the Bootstrap plugins. We could also consider implementing our
 own popover functionality, perhaps using Popper.
 -->
 <template>
-  <div class="popover-content-source">
+  <div class="popover-content-source" aria-hidden="true">


Because we are now displaying this off-screen rather than hiding it with display: none, we tell screen readers to ignore it.

matthew-white · 2024-12-09T07:05:10Z

src/components/dataset/show.vue

@@ -84,7 +83,7 @@ export default {
    const { tabPath, tabClass } = useTabs(datasetPath());
    return {
      project, dataset, ...resourceStates([project, dataset]),
-      projectPath, datasetPath, tabPath, tabClass, canRoute


I searched for all uses of datasetPath() to see where to add DatasetLink. I noticed that this component doesn't use datasetPath() in the template, so it doesn't need to return it from setup().

matthew-white · 2024-12-09T07:13:49Z

src/components/project/form-row.vue

      </template>
      <template v-else-if="canLinkToSubmissions">
-        <router-link :to="submissionsPath.all">{{ form.nameOrId }}</router-link>
+        <form-link :form="form" :to="submissionsPath.all"/>


canRoute() and LinkIfCan don't always work that well on the homepage, which is why <router-link> + v-if was used here. For example, form pages will check the form resource, but the form resource isn't set on the homepage, so it will never fail validateData when checked from the homepage.

Here, the default value of the to prop is primaryFormPath(). However, given the behavior of canRoute() on the homepage, primaryFormPath() won't ever return a path to the submissions page. That's why we specify submissionsPath.all explicitly.

matthew-white · 2024-12-09T07:15:10Z

src/composables/event-listener.js

I've wanted to make this change on other occasions as well. There are times when it's convenient to pass a template ref to useEventListener().

matthew-white · 2024-12-09T07:17:23Z

src/request-data/submission.js

-        ? meta.instanceName
-        : submission.__id;
-    })
+      if (meta == null || typeof meta !== 'object') return null;


I thought I might as well check that meta is an object as well.

sadiqkhoja

I am still reviewing this PR, wanted to share my observation up till now:

1- Hover card should be 3-4px above the viewport: "If there isn’t room onscreen (without needing to scroll) to fit the height of the card, I should see the bottom of the card 3-4px above the bottom of the browser viewport" getodk/central#797

2- What do you think of caching the response of hover requests until user navigates to another page/route? right now every time I hover on a link a request is sent.

3- If I create a new Submission in a separate tab and then hover over Form link on the project overview page, it shows the correct number of Submission count but count is not updated on the FormList table.

None of the above are super critical to get this PR merged.

sadiqkhoja · 2024-12-09T23:21:15Z

src/composables/hover-card.js

+      timeoutId = null;
+    }
+  };
+  useEventListener(anchorRef, 'mouseleave', () => {


Nice to have: I wish if hover cards don't hide on mouse leaving the anchor link and entering into the hover card.

One way to achieve that is to append popover inside the anchor instead of 'body' (src/components/popover.vue:87).

I like this idea. If the user could enter the hover card, then we could show tooltips over truncated text. That said, the release criteria says the opposite:

I should not be able to hover on or interact with the infocard.

How about we discuss more in the meeting tomorrow?

We decided that we wouldn't make this change for v2024.3, but I'm going to file a follow-up issue about this.

matthew-white · 2024-12-10T03:44:28Z

3- If I create a new Submission in a separate tab and then hover over Form link on the project overview page, it shows the correct number of Submission count but count is not updated on the FormList table.

I actually think this is sort of a nice thing. It's a signal that the page you're viewing is out of date. We could patch the one form, but it might not be the only stale data on the page.

For what it's worth, I've noticed that GitHub does something similar with their hover cards. For example:

Navigate to the list of open PRs.
Open a PR in a new tab.
Merge the PR.
In the list of PRs, hover over the PR. The hover card will show that the PR is merged. However, the PR will not change in the list itself.

2- What do you think of caching the response of hover requests until user navigates to another page/route? right now every time I hover on a link a request is sent.

I like this idea! I don't think I'll have time to do it before release, but I've filed an issue for it and slotted it for the next release: #1093.

Since I mentioned them already, I'll say that I also noticed that GitHub does cache their hover cards. For example:

Navigate to the list of open PRs.
Hover over a PR.
Open the PR in a new tab.
Merge the PR.
In the list of PRs, hover over the PR. The hover card will show that the PR is still open.

sadiqkhoja

This is amazing 🎉

Liked the approach of calculating and positioning hover-card, it's not easy.

Test coverage is also good 👍

src/container/hover-card.js

src/composables/hover-card.js

src/request-data/hover-card.js

sadiqkhoja · 2024-12-10T14:43:20Z

src/components/hover-card/submission.vue

+const idParts = computed(() => {
+  const id = submission.__id;
+  const match = id.match(/^((?:uuid:)?[0-9a-f]{8})[0-9a-f-]+([0-9a-f]{8})$/);
+  return match != null ? [match[1], '…', match[2]] : [id];
+});


Some unit tests for this would be helpful. We can add them later

When you say unit tests, are you thinking that we should move this logic into a utility function somewhere? I'd be happy to do that.

There are some tests of this behavior in test/components/hover-card/submission.spec.js. There are a couple of cases that I didn't test:

The case where the instance ID doesn't start with uuid:, in which case it should still be truncated.

The case where the instance ID is not a UUID, in which case idParts.value falls back to [id].

If I added tests of those cases in test/components/hover-card/submission.spec.js, would that be enough? Or should I create a utility function and test that separately? I'd be happy with either approach.

Thinking of the future, I bet that there will be other cases in which we want to truncate instance IDs in the middle like this. When that day comes, I'll probably move some of this into its own small component.

I find regex harder to read, seeing unit (or other) tests assure me that it is doing what it suppose to do + tests are kind of requirement documentation as well.

sadiqkhoja · 2024-12-10T15:02:57Z

src/components/hover-card.vue

+
+const width = ref('');
+const el = ref(null);
+const resize = () => {


I'd like this function to be cross-browser tested

Sounds good. 👍 I think it'd be difficult to explain some of this logic to the QA team, but I could test it out locally. I usually work in Chrome, but it'd be easy for me to test it in Firefox. I can leave a comment about the scenarios I try out: there are a few specific ones that I have in mind.

I think QA team uses Firefox, they will report if they see something strange. And I try in Safari

src/components/popover.vue

matthew-white · 2024-12-11T22:37:50Z

I want to write a few more tests, especially of HoverCardEntity.

I still need to finish these tests. I'll file a follow-up issue about that.

matthew-white mentioned this pull request Nov 26, 2024

Add "Last update" to hover card for entity list getodk/central#803

Open

matthew-white force-pushed the hover-cards branch from 346ee43 to b84a874 Compare December 3, 2024 20:38

This was referenced Dec 4, 2024

Add entityDetails computed property to SubmissionFeedEntry #1073

Merged

Change .dl-horizontal to be a grid #1077

Merged

matthew-white added 5 commits December 9, 2024 00:39

Set up hover cards

fd11ef8

Add hover card to show an entity

79d2de9

Add hover card to show an entity list

e338e52

Add hover card to show a form

1f040c7

Add hover card to show a submission

898aa14

matthew-white force-pushed the hover-cards branch from b84a874 to 898aa14 Compare December 9, 2024 05:40

matthew-white requested a review from sadiqkhoja December 9, 2024 07:39

matthew-white marked this pull request as ready for review December 9, 2024 07:39

matthew-white commented Dec 9, 2024

View reviewed changes

matthew-white mentioned this pull request Dec 9, 2024

Move files to src/container/ #1088

Open

sadiqkhoja reviewed Dec 9, 2024

View reviewed changes

matthew-white mentioned this pull request Dec 10, 2024

Cache hover card responses #1093

Open

Merge branch 'master' into hover-cards

5e95312

sadiqkhoja approved these changes Dec 10, 2024

View reviewed changes

Make small changes related to hover cards

12afaf1

matthew-white merged commit dbf6458 into master Dec 11, 2024
1 check passed

matthew-white deleted the hover-cards branch December 11, 2024 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hover cards #1067

Add hover cards #1067

matthew-white commented Nov 26, 2024 •

edited

Loading

matthew-white commented Dec 9, 2024

matthew-white left a comment

matthew-white Dec 9, 2024

sadiqkhoja Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

matthew-white Dec 9, 2024

sadiqkhoja left a comment •

edited

Loading

sadiqkhoja Dec 9, 2024

matthew-white Dec 10, 2024

matthew-white Dec 11, 2024

matthew-white commented Dec 10, 2024

sadiqkhoja left a comment

sadiqkhoja Dec 10, 2024

matthew-white Dec 11, 2024

sadiqkhoja Dec 12, 2024

sadiqkhoja Dec 10, 2024

matthew-white Dec 11, 2024

sadiqkhoja Dec 12, 2024

matthew-white commented Dec 11, 2024

Add hover cards #1067

Add hover cards #1067

Conversation

matthew-white commented Nov 26, 2024 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Before submitting this PR, please make sure you have:

matthew-white commented Dec 9, 2024

matthew-white 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

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

sadiqkhoja left a comment • edited Loading

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

matthew-white commented Dec 10, 2024

sadiqkhoja 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

Choose a reason for hiding this comment

matthew-white commented Dec 11, 2024

matthew-white commented Nov 26, 2024 •

edited

Loading

sadiqkhoja left a comment •

edited

Loading