search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 442 forks source link

Migrate declarative use of Modal to openModal #9992

Open jardakotesovec opened 1 month ago

jardakotesovec commented 1 month ago

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: https://github.com/pkp/ojs/pull/4339 omp: https://github.com/pkp/omp/pull/1608 ops: https://github.com/pkp/ops/pull/717 ui-library: https://github.com/pkp/ui-library/pull/366 pkp-lib: https://github.com/pkp/pkp-lib/pull/10124

jardakotesovec commented 3 days ago

@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!

ewhanson commented 1 day ago

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