Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate declarative use of Modal to openModal #9992

Closed
jardakotesovec opened this issue May 29, 2024 · 3 comments
Closed

Migrate declarative use of Modal to openModal #9992

jardakotesovec opened this issue May 29, 2024 · 3 comments
Assignees
@jardakotesovec
Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience
Milestone
3.5 Internal

Comments

@jardakotesovec
Copy link
Contributor

jardakotesovec commented May 29, 2024 •
edited
Loading

Describe the issue
When migrated to vue3 - simple declarative way to control modals was used - example . These cases can be easily found by searching for <modal> tag. Since than we moved to centrally controlled modals, as it allowed to consolidate old and new modals to appear the same, allows to combine legacy and new ones and have correct behaviour when closing etc.

Another reason to use centrally controlled modals is to compensate for headless-ui limitation, which currently requires to define modals in nested fashion to work correctly. Which does not work very well for cases when the modal needs to be opened sometime on first level but sometime also on top another modal. Typical use case is to display modal with network error, when the communication with API fails.

Solution
Migrate existing <modal> declaration to use of openModal from useModal composable. This would require to define modals as separate components. If that would make the transition too cumbersome, as alternative its possible to create <ModalInline> component, that would still let the component define inline, but it still would be controlled via openModal using the modal name.

What application are you using?
OJS, OMP or OPS version main

PRs:
ojs: pkp/ojs#4339
omp: pkp/omp#1608
ops: pkp/ops#717
ui-library: pkp/ui-library#366
pkp-lib: #10124
@jardakotesovec jardakotesovec added Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience labels May 29, 2024
@jardakotesovec jardakotesovec added this to the 3.5 Internal milestone May 29, 2024
@jardakotesovec jardakotesovec self-assigned this May 29, 2024
@jardakotesovec jardakotesovec mentioned this issue May 29, 2024
Merged
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 20, 2024
@jardakotesovec
pkp#9992 Migration to side modal for context stats
ded74ba
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 20, 2024
@jardakotesovec
pkp#9992 Migration to side modal for publication stats
29aef81
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate Announcements to SideModal
b346da4
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 clean up announcements moved to SideModal
a85984d
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 Composer modals migrated to SideModal
cabeb6f
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 FileAttacher modal clean up
62dabec
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate issue stats modal to SideModal
be8d35c
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 publication stats migrated to side modal
9b48b5b
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 users stats export modal migrated to side modal
fcec29a
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 wizard modal migrated to side modal
df2f883
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 FieldPreparedContent migrated to side modal
3e5b62c
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 InstitutionsListPanel migrated to side modal, make s…
53714f8 
…ure that the form errors are propagated to the modal for other ListPanels
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate SubmissionFilesEditModal to openSideModal
c0b07f0
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 HighlightsListPanel migrated to side modal
48af3d4
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 20, 2024
@jardakotesovec
pkp#9992 users stats export modal migrated to side modal
0ceb1bb
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 20, 2024
@jardakotesovec
pkp#9992 wizard modal migrated to side modal
29ae8f6
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 20, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate ContributorsListPanel to side panel, rename …
e80e4c9 
…some other modals for more consistent naming
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 20, 2024
@jardakotesovec
pkp#9992 Migrate ContributorsListPanel to side panel, migrated transl…
da832a1 
…ations
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 24, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate CatalogListPanel to side modal
634c90d
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 24, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate doi list panel to side modal
d90bec7
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 24, 2024
@jardakotesovec
pkp/pkp-lib#9992 migrate workflow page to side modal
a56f81c
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 24, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate issue stats modal to side modal
43d0439
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 24, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate workflow page modal to side modal
d5700d8
@jardakotesovec jardakotesovec mentioned this issue Jun 24, 2024
Closed
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 migrate manageEmails to side modal
e2081d9
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 migrate manageEmails to side modal
abd7b89
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate error doi message modals to useDialog
019458e
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 clean up modals in stories, copied over the tpl files
d5ed9ff
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 move title directly to modal component when its not …
3632001 
…dynamic
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Update documentation for SideModal and Dialog, refac…
83320bc 
…tor Dialog to expose the props for docs
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Clean up
939ecf4
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate issue stats modal to side modal
80b8ead
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Migrate workflow page modal to side modal
4877770
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Update locale keys
18be233
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Remove legacy modal from workflow.tpl as its migrate…
dab138e 
…d to openSideModal
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Fix SideModal stories template
5a77c55
jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 25, 2024
@jardakotesovec
pkp#9992 adjust cypress tests for side modal
ed49abe
jardakotesovec added a commit to jardakotesovec/ojs that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 adjust cypress tests for side modal
40c4e14
jardakotesovec added a commit to jardakotesovec/omp that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Cypress tests update for side modal
3f40d98
jardakotesovec added a commit to jardakotesovec/ops that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Update cypress tests for side modal
6c079b1
@jardakotesovec
Copy link
Contributor Author

jardakotesovec commented Jun 25, 2024 •
edited
Loading

@ewhanson May I ask you for code review on this one?

Its one of these annoying PRs, where I migrated probably more than 20 modals. I did test each of them individually. Hopefully fixed more bugs than introduced along the way. There were some mistakes from the vue3 migration.

So apart from just scanning through if you can spot something silly - could you specifically tests Doi functionality that you are very familiar with? The trivial modals which just show some error feedback I migrated to use openDialog, and the more complex one to openSideModal.

I also added more documentation for Dialog and SideModal to document better the Dialog Props and purpose.

I am still waiting for OPS/OMP tests to pass - they still might need couple of cypress tests tweaks.. but OJS are passing now, so I think its good time to get into it.

Thanks!

jardakotesovec added a commit to jardakotesovec/pkp-lib that referenced this issue Jun 25, 2024
@jardakotesovec
pkp#9992 Adjust cypress tests for side modal migration
978584a
jardakotesovec added a commit to jardakotesovec/ops that referenced this issue Jun 25, 2024
@jardakotesovec
pkp/pkp-lib#9992 Update cypress tests for side modal
dcda271
@ewhanson
Copy link
Collaborator

Thanks @jardakotesovec. There's a lot of really good stuff here! 👍 Only a few minor comments and questions. Otherwise, looks good!

jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jul 2, 2024
@jardakotesovec
pkp/pkp-lib#9992 Address code review feedback
ca0ac83
jardakotesovec added a commit to pkp/ui-library that referenced this issue Jul 2, 2024
@jardakotesovec
I9992 (#366)
76ca2af 
* pkp/pkp-lib#9992 Migrate Announcements to SideModal

* pkp/pkp-lib#9992 clean up announcements moved to SideModal

* pkp/pkp-lib#9992 Composer modals migrated to SideModal

* pkp/pkp-lib#9992 FileAttacher modal clean up

* pkp/pkp-lib#9992 Migrate issue stats modal to SideModal

* pkp/pkp-lib#9992 publication stats migrated to side modal

* pkp/pkp-lib#9992 users stats export modal migrated to side modal

* pkp/pkp-lib#9992 wizard modal migrated to side modal

* pkp/pkp-lib#9992 FieldPreparedContent migrated to side modal

* pkp/pkp-lib#9992 InstitutionsListPanel migrated to side modal, make sure that the form errors are propagated to the modal for other ListPanels

* pkp/pkp-lib#9992 Migrate SubmissionFilesEditModal to openSideModal

* pkp/pkp-lib#9992 HighlightsListPanel migrated to side modal

* pkp/pkp-lib#9992 Migrate ContributorsListPanel to side panel, rename some other modals for more consistent naming

* pkp/pkp-lib#9992 Migrate CatalogListPanel to side modal

* pkp/pkp-lib#9992 Migrate doi list panel to side modal

* pkp/pkp-lib#9992 migrate workflow page to side modal

* pkp/pkp-lib#9992 migrate manageEmails to side modal

* pkp/pkp-lib#9992 Migrate error doi message modals to useDialog

* pkp/pkp-lib#9992 clean up modals in stories, copied over the tpl files

* pkp/pkp-lib#9992 move title directly to modal component when its not dynamic

* pkp/pkp-lib#9992 Update documentation for SideModal and Dialog, refactor Dialog to expose the props for docs

* pkp/pkp-lib#9992 Clean up

* pkp/pkp-lib#9992 Fix SideModal stories template

* pkp/pkp-lib#9992 Address code review feedback
jardakotesovec added a commit that referenced this issue Jul 2, 2024
@jardakotesovec
I9992 (#10124)
cfb1849 
* #9992 Migration to side modal for context stats

* #9992 Migration to side modal for publication stats

* #9992 users stats export modal migrated to side modal

* #9992 wizard modal migrated to side modal

* #9992 Migrate ContributorsListPanel to side panel, migrated translations

* #9992 migrate manageEmails to side modal

* #9992 adjust cypress tests for side modal

* #9992 Adjust cypress tests for side modal migration
jardakotesovec added a commit to pkp/ojs that referenced this issue Jul 2, 2024
@jardakotesovec
I9992 (#4339)
6eac08d 
* pkp/pkp-lib#9992 Migrate issue stats modal to side modal

* pkp/pkp-lib#9992 Migrate workflow page modal to side modal

* pkp/pkp-lib#9992 Update locale keys

* pkp/pkp-lib#9992 adjust cypress tests for side modal
jardakotesovec added a commit to pkp/omp that referenced this issue Jul 2, 2024
@jardakotesovec
I9992 (#1608)
9d82806 
* pkp/pkp-lib#9992 Remove legacy modal from workflow.tpl as its migrated to openSideModal

* pkp/pkp-lib#9992 Cypress tests update for side modal
jardakotesovec added a commit to pkp/ops that referenced this issue Jul 2, 2024
@jardakotesovec
pkp/pkp-lib#9992 Update cypress tests for side modal (#717)
9cf55bb
@jardakotesovec
Copy link
Contributor Author

@ewhanson Thank you for the review.. issues addressed and its merged now. Two issues related to linting I will look into separately.

jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jul 2, 2024
@jardakotesovec
pkp/pkp-lib#9992 Bug fixes for side modal migration
018168e
jardakotesovec added a commit to jardakotesovec/ui-library that referenced this issue Jul 2, 2024
@jardakotesovec
pkp/pkp-lib#9992 Bug fixes for side modal migration
f460c3b
jardakotesovec added a commit to pkp/ui-library that referenced this issue Jul 2, 2024
@jardakotesovec
pkp/pkp-lib#9992 Bug fixes for side modal migration (#376)
757e188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jardakotesovec jardakotesovec

Labels
Housekeeping:1:Todo Any dependency management or refactor that would be nice to have some day. UI/UX Issues affecting the user interface/user experience
Projects
None yet
Milestone
3.5 Internal
Development

No branches or pull requests
2 participants
@jardakotesovec @ewhanson