Closed lognaturel closed 3 months ago
@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:
user.create
, form.create
, submission.list
, submission.create
).Should I do a quick first round of code review now?
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.
Putting this one in your court @matthew-white with two todos I can't quickly figure out!
loggedIn
between Navbar and App. I don't feel like I know where it should go.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.
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)
I approve of and appreciate your updates, @matthew-white! Looks good to go to me.
Closes getodk/central#616
Opening this as draft so the new component can be reviewed and the general approach can be discussed.
Design
Proposed plan
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:
npm run test
andnpm run lint
and confirmed all checks still pass OR confirm CircleCI build passes