Add feedback button #962

lognaturel · 2024-03-21T00:19:15Z

Closes getodk/central#616

Opening this as draft so the new component can be reviewed and the general approach can be discussed.

Design

Button is intended to be easy to access but not distracting
- Initially had magenta color but @alyblenkin and I paired and worked to make it less eye-catching
- Position is fixed at the right and almost the bottom. It's intended to be of the way but still easy to click
- Almost all sizes are relative so it can be scaled up or down as the rest of the design changes
Form is intended to be short and easy to fill out while providing nudges on the kind of feedback we want
- Captures host and current generic page name (no form titles, instance ids, etc)

Proposed plan

Merge in only the feedback button component
Apply the changes to use the button on ODK Cloud only initially; self-hosters could choose to do the same if they want; we could even check for .getodk.cloud in the hostname
See what kind of feedback we get, consider always adding it and/or making it configurable

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

Manual verification only. Made some test submissions.

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

I considered doing this as a blob of markup we'd paste in just for ODK Cloud users initially or keeping the component private. However, I think it's simpler to have it in the code base. We don't particularly mind if self-hosters want to enable this using our public survey or their own -- we just want to roll it out to Cloud customers only initially to test out the idea ("meta-feedback").

One possible downside of this approach is that someone ill-intentioned could use the public form to spam us. We have mitigations in place for that and worse case we could close the form.

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?

The feedback button component is new so should be very low risk. We need to decide exactly how we want to enable it for Cloud and that could carry some risk because it has logic.

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

I don't think so.

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-03-26T18:56:20Z

@lognaturel's first Composition API component, woohoo! 🎉

Merge in only the feedback button component

Apply the changes to use the button on ODK Cloud only initially; self-hosters could choose to do the same if they want; we could even check for .getodk.cloud in the hostname

I think the easiest thing would be to use src/config.js to configure whether the button is shown. We similarly use src/config.js for the customizations on the homepage. Trying to apply changes to App would probably lead to conflicts in the future, since we modify that component from time to time.

I considered doing this as a blob of markup we'd paste in just for ODK Cloud users initially or keeping the component private. However, I think it's simpler to have it in the code base.

That makes sense to me to have it in the code base.

Almost all sizes are relative so it can be scaled up or down as the rest of the design changes

Almost none of our other sizes are relative, in part because Bootstrap 3 didn't use relative sizes. No harm in using relative sizes here though!

Other ideas for data that Frontend could pass to the form:

Central version. This would be especially useful if we decide to roll out the button outside of ODK Cloud.
User role/verbs. The user's role determines what is shown to the user, so this would provide us information about what the user is actually seeing.
- Implementation detail: Frontend doesn't know the user's role, only their verbs. Frontend could pass along all the verbs that the user has been granted or just a subset that would allow us to infer the role (user.create, form.create, submission.list, submission.create).
When was the user's account created? It could be helpful to know if it's a new user getting to know Central or an experienced user providing feedback. We use this timestamp in another place, to determine whether to show "Help improve Central!" in the navbar. (We start showing it two weeks after the account was created.)

Should I do a quick first round of code review now?

lognaturel · 2024-03-26T19:29:03Z

use src/config.js to configure whether the button is shown

Ah, yes, sure!

Other ideas for data that Frontend could pass to the form

These are all great ideas. How about we see what kind of feedback we get and add accordingly. I'd prefer to capture the minimum amount of information we need to make good decisions. My first reaction was to get excited about all of these but they're kind of on the edge of PII and if we don't have to collect them to get value, maybe it's better that we don't.

Should I do a quick first round of code review now?

Please! I imagine it will be fast.

lognaturel · 2024-04-17T23:27:33Z

Putting this one in your court @matthew-white with two todos I can't quickly figure out!

Write test for anonymous user cases (there's one that's broken and I guess we might as well add the final case)
Share loggedIn between Navbar and App. I don't feel like I know where it should go.

matthew-white

I wrote some comments about the previous commit f229b26, and @lognaturel and I reviewed them interactively. A lot of the comments have been resolved already, but I thought it'd help to make them visible on the PR. I'll work on making those remaining small changes, then I'll merge. 🚀

src/components/feedback-button.vue

src/components/app.vue

src/components/feedback-button.vue

src/components/app.vue

src/components/feedback-button.vue

src/config.js

matthew-white · 2024-04-21T19:25:44Z

I'm noticing an issue whereby if a modal has a scrollbar, the feedback button overlays it:

Most of our modals don't scroll, but some do, including the new entity bulk upload modal. Other examples are AnalyticsPreview and (if the entity list has enough properties) EntityUpdate.

I don't see an easy way to fix this without reworking FeedbackButton a little. If the z-index of the button is greater than the z-index of the modal, than under the correct approach, it will overlay the scrollbar. If the z-index of the button is less than the z-index of the modal, then it won't be possible to interact with the button: clicking it will have no effect and will just close the modal. To fix the issue, maybe we should keep the z-index as it is, but position the feedback button exactly to the left of the scrollbar. Right now, the feedback button is larger than what is visible, but that would need to change under this approach: we would change the hover effect so that it causes the button to grow rather than repositioning it.

A different, cheaper way to address the issue would be to hide the feedback button entirely when a modal is open (hide #feedback-button if it is a descendant of body.modal-open). A little harder but also possible would be to hide the feedback button just if an open modal has a scrollbar. We could also rework FeedbackButton as described above. I'm going to leave things alone for now, but I wanted to make a note of the issue in case we want to address it.

Also, while we're on the subject of modals, I wanted to mention a smaller issue. When a modal is open, it's not possible to use the tab key to reach the feedback button: you have to click on it.

lognaturel · 2024-04-22T03:18:01Z

What a literal edge case!

I would tend to leave it as-is for now and file an issue. I can look into growing the button instead of moving it while regression testing is ongoing but I also think it would be ok to release as-is.

This is primarily based on the belief that it's rare for folks to use the actual scrollbar. I'm sure there are good accessibility reasons to do so in which case it would still be possible to use unless the screen is extremely short or there's a huge amount of content in the modal. ~~I also believe that this only affects the analytics summary and the new entity upload modal. Is that right?~~ (I see you addressed this)

lognaturel · 2024-04-24T23:19:53Z

test/components/app.spec.js

@@ -249,7 +249,7 @@ describe('App', () => {
        config: { showsFeedbackButton: true }
      };

-      const app = await load('/', { container });
+      const app = await load('/login', { container }).restoreSession(false);


I am annoyed! This is different from loading '/' with restoreSession(false)? I'm pretty sure I had tried that.

They should be the same! restoreSession(false) is the key part. I just changed it to /login because navigating to / when you're logged out will send you to /login. Might as well go there right away haha.

lognaturel · 2024-04-24T23:21:32Z

src/util/session.js

  });
+
+  /* visiblyLoggedIn.value is `true` if the user not only has all the data from


lognaturel · 2024-04-24T23:22:15Z

I approve of and appreciate your updates, @matthew-white! Looks good to go to me.

matthew-white · 2024-04-24T23:17:35Z

src/util/session.js

+  const visiblyLoggedIn = computed(() => currentUser.dataExists &&
+    router.currentRoute.value !== START_LOCATION &&
+    router.currentRoute.value.path !== '/login');


I adapted visiblyLoggedIn from the loggedIn computed property in Navbar. I also added a check that the current route isn't START_LOCATION so that visiblyLoggedIn.value is false throughout the initial navigation. The initial navigation is async because it attempts to restore the session. During the navigation, we don't show the navbar, and this additional check means that the feedback button also won't be shown.

matthew-white · 2024-04-24T23:19:54Z

test/components/navbar/actions.spec.js

-  it('indicates if the user is not logged in', () => {
-    const navbar = mount(Navbar, {
-      container: { router: mockRouter('/login') },
-      global: {
-        // Stubbing AnalyticsIntroduction because of its custom <router-link>
-        stubs: { AnalyticsIntroduction: true }
-      }
-    });
-    const text = navbar.getComponent(NavbarActions).get('a').text();
-    text.should.equal('Not logged in');
-  });
+  it('indicates if the user is not logged in', () =>
+    load('/login')


I also could have patched the test by injecting visiblyLoggedIn. However, I think it's a better test to call load() and mount App. That way, the test will actually use visiblyLoggedIn as provided by App.

matthew-white · 2024-04-24T23:28:49Z

test/components/app.spec.js

@@ -249,7 +249,7 @@ describe('App', () => {
        config: { showsFeedbackButton: true }
      };

-      const app = await load('/', { container });
+      const app = await load('/login', { container }).restoreSession(false);


They should be the same! restoreSession(false) is the key part. I just changed it to /login because navigating to / when you're logged out will send you to /login. Might as well go there right away haha.

lognaturel added 2 commits March 20, 2024 17:24

Add feedback button

7869e40

Don't show feedback button on login screen

f229b26

lognaturel force-pushed the feedback-button branch from 86b6fd3 to f229b26 Compare March 21, 2024 00:24

Respond to review feedback

5dc81c5

matthew-white reviewed Apr 19, 2024

View reviewed changes

src/config.js Outdated Show resolved Hide resolved

matthew-white added 5 commits April 24, 2024 16:20

Resolve lint errors

6b8e0fd

Fix test of FeedbackButton

fa7c8cd

Change default value of showsFeedbackButton to false

1bbb612

useSessions(): return whether user is visibly logged in

93480a9

Fix focus animation

2ec741f

matthew-white marked this pull request as ready for review April 24, 2024 23:09

lognaturel commented Apr 24, 2024

View reviewed changes

src/util/session.js

});

/* visiblyLoggedIn.value is `true` if the user not only has all the data from

Copy link

Member Author

lognaturel Apr 24, 2024

Choose a reason for hiding this comment

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

Ok, nice!

lognaturel requested a review from matthew-white April 24, 2024 23:22

matthew-white approved these changes Apr 24, 2024

View reviewed changes

matthew-white merged commit e75829a into getodk:master Apr 24, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add feedback button #962

Add feedback button #962

lognaturel commented Mar 21, 2024 •

edited

Loading

matthew-white commented Mar 26, 2024

lognaturel commented Mar 26, 2024

lognaturel commented Apr 17, 2024 •

edited by matthew-white

Loading

matthew-white left a comment

matthew-white commented Apr 21, 2024

lognaturel commented Apr 22, 2024 •

edited

Loading

lognaturel Apr 24, 2024

matthew-white Apr 24, 2024

lognaturel Apr 24, 2024

lognaturel commented Apr 24, 2024

matthew-white Apr 24, 2024

matthew-white Apr 24, 2024

matthew-white Apr 24, 2024

		});

		/* visiblyLoggedIn.value is `true` if the user not only has all the data from

Add feedback button #962

Add feedback button #962

Conversation

lognaturel commented Mar 21, 2024 • edited Loading

Design

Proposed plan

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?

Does this change require updates to user documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

matthew-white commented Mar 26, 2024

lognaturel commented Mar 26, 2024

lognaturel commented Apr 17, 2024 • edited by matthew-white Loading

matthew-white left a comment

Choose a reason for hiding this comment

matthew-white commented Apr 21, 2024

lognaturel commented Apr 22, 2024 • edited Loading

lognaturel Apr 24, 2024

Choose a reason for hiding this comment

matthew-white Apr 24, 2024

Choose a reason for hiding this comment

lognaturel Apr 24, 2024

Choose a reason for hiding this comment

lognaturel commented Apr 24, 2024

matthew-white Apr 24, 2024

Choose a reason for hiding this comment

matthew-white Apr 24, 2024

Choose a reason for hiding this comment

matthew-white Apr 24, 2024

Choose a reason for hiding this comment

lognaturel commented Mar 21, 2024 •

edited

Loading

lognaturel commented Apr 17, 2024 •

edited by matthew-white

Loading

lognaturel commented Apr 22, 2024 •

edited

Loading