Use a modal for license explanation #4800

obulat · 2024-08-22T11:28:04Z

Fixes

Fixes #3582 by @fcoveram
Fixes #4802 by @fcoveram

Description

This PR converts the license explanation popover to a modal.

Desktop

Mobile

Functionality changes

On mobile screens, the license explanation modal appears on top of the content settings modal. So, when the user clicked outside of the license explanation modal, the lower content settings modal handled this event and closed, keeping the top-most license explanation one open.
To correctly handle this, I added a use-dialog-stack composable and ids to all modals and popovers to keep track and route the event to the currently open, top-most dialog.

Vertical overflow

I added the max-height to modals, set to the full height of the screen, minus the height of the modal header, minus an arbitrary value of 6rem. @fcoveram, what is the best value for vertical margin of the modal?

Testing Instructions

Run the app and go to http://localhost:8443/search?q=cat.
On desktop, open the filters sidebar and click on one of the ? buttons next to licenses. The modal should appear. It's styles should match the mockups, and it should function correctly.
On a mobile, open the content settings modal, select the Filters tab and click on one of the ? buttons next to licenses. The modal should appear. Clicking on the close button, pressing Escape or clicking outside of the modal should close it, and only it (not the content settings modal below it).

Checklist

My pull request has a descriptive title (not a vague title likeUpdate 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.
I ran the DAG documentation generator (ov just catalog/generate-docs for catalog
PRs) or the media properties generator (ov just catalog/generate-docs media-props
for the catalog or ov just api/generate-docs for the API) where applicable.

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.

obulat · 2024-08-23T08:11:03Z

frontend/src/components/VContentReport/VContentReportModal.vue

@@ -51,7 +52,7 @@ const close = () => {
    <template #default>
      <VContentReportForm
        ref="contentReportFormRef"
-        class="p-7 pt-0 sm:p-9"
+        class="p-7 pt-0 sm:p-9 sm:pt-0"


This is the fix for #4802

zackkrida · 2024-08-23T14:42:59Z

frontend/src/components/VPopover/VPopover.vue

@@ -15,6 +15,7 @@ import type { Placement, Strategy } from "@floating-ui/dom"

 const props = withDefaults(
  defineProps<{
+    id: string


Suggested change

id: string

/**

* The id used to keep track of the modal in the open modal stack, to enable

* nested modals.

*/

id: string

Should this have a similar comment?

zackkrida · 2024-08-23T14:45:26Z

frontend/src/composables/use-dialog-stack.ts

+  const activeDialog = computed(() => stack.value[stack.value.length - 1])
+
+  return {
+    stack,
+    push,
+    pop,
+    clear,
+    indexOf,
+    activeDialog,
+  }
+}


Can we document what activeDialog means in terms of the actual UI here? For example saying it's the "top-level dialog with no dialogs above it" or something. That's a terrible example but hopefully conveys what I mean.

zackkrida

I love the ID stack approach to fixing the nested dialog issue! Everything I've tested works great. Some documentation nits, otherwise LGTM.

Signed-off-by: Olga Bulat <[email protected]>

openverse-bot added 🧱 stack: frontend Related to the Nuxt frontend 🟨 priority: medium Not blocking but should be addressed soon ✨ goal: improvement Improvement to an existing user-facing feature 🖼️ aspect: design Concerns related to design labels Aug 22, 2024

obulat force-pushed the add/modal-for-license-explanation branch from 3d87622 to ac27177 Compare August 22, 2024 11:37

obulat changed the base branch from main to script-setup/popoverContent-licenseElements-searchTypePopover August 22, 2024 11:38

obulat mentioned this pull request Aug 22, 2024

Convert components for license explanation modal #4801

Merged

8 tasks

obulat force-pushed the script-setup/popoverContent-licenseElements-searchTypePopover branch from 53c9bfe to cc2c019 Compare August 23, 2024 04:13

obulat force-pushed the add/modal-for-license-explanation branch 2 times, most recently from 814496e to 54ec74f Compare August 23, 2024 05:02

obulat linked an issue Aug 23, 2024 that may be closed by this pull request

Big white space in the modal of content report #4802

Closed

openverse-bot added 💻 aspect: code Concerns the software code in the repository 🕹 aspect: interface Concerns end-users' experience with the software labels Aug 23, 2024

obulat requested a review from fcoveram August 23, 2024 06:18

obulat self-assigned this Aug 23, 2024

obulat marked this pull request as ready for review August 23, 2024 08:10

obulat requested a review from a team as a code owner August 23, 2024 08:10

obulat requested review from zackkrida and dhruvkb August 23, 2024 08:10

obulat commented Aug 23, 2024

View reviewed changes

obulat removed the 🖼️ aspect: design Concerns related to design label Aug 23, 2024

openverse-bot added the 🖼️ aspect: design Concerns related to design label Aug 23, 2024

zackkrida reviewed Aug 23, 2024

View reviewed changes

zackkrida approved these changes Aug 23, 2024

View reviewed changes

Base automatically changed from script-setup/popoverContent-licenseElements-searchTypePopover to main August 23, 2024 14:59

obulat force-pushed the add/modal-for-license-explanation branch from f8eb161 to cfffa98 Compare August 23, 2024 16:17

obulat added 3 commits August 24, 2024 07:32

Use a modal for license explanation

dc29e76

Signed-off-by: Olga Bulat <[email protected]>

Fix content report padding

58cec70

Signed-off-by: Olga Bulat <[email protected]>

Add id to unit tests

5cd7d25

Signed-off-by: Olga Bulat <[email protected]>

obulat added 6 commits August 24, 2024 07:32

Make modal scrollable when overflowing

7915962

Signed-off-by: Olga Bulat <[email protected]>

Abstract the composable to handle popovers

dc78f43

Signed-off-by: Olga Bulat <[email protected]>

Use constants for ids

74d5830

Signed-off-by: Olga Bulat <[email protected]>

Remove negative margin from external search form

ebee0cc

Signed-off-by: Olga Bulat <[email protected]>

Update external sources modal snapshots

21be954

Signed-off-by: Olga Bulat <[email protected]>

Add doc comments

9dcafec

Signed-off-by: Olga Bulat <[email protected]>

obulat force-pushed the add/modal-for-license-explanation branch from f61532d to 9dcafec Compare August 24, 2024 04:33

obulat merged commit bc81c65 into main Aug 24, 2024
43 checks passed

obulat deleted the add/modal-for-license-explanation branch August 24, 2024 04:49

obulat mentioned this pull request Sep 30, 2024

Content report popover appears over the pages modal #569

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a modal for license explanation #4800

Use a modal for license explanation #4800

obulat commented Aug 22, 2024 •

edited

Loading

obulat Aug 23, 2024

zackkrida Aug 23, 2024

zackkrida Aug 23, 2024 •

edited

Loading

zackkrida left a comment

Use a modal for license explanation #4800

Use a modal for license explanation #4800

Conversation

obulat commented Aug 22, 2024 • edited Loading

Fixes

Description

Desktop

Mobile

Functionality changes

Vertical overflow

Testing Instructions

Checklist

Developer Certificate of Origin

obulat Aug 23, 2024

Choose a reason for hiding this comment

zackkrida Aug 23, 2024

Choose a reason for hiding this comment

zackkrida Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

zackkrida left a comment

Choose a reason for hiding this comment

obulat commented Aug 22, 2024 •

edited

Loading

zackkrida Aug 23, 2024 •

edited

Loading