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.

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

Reduce duplicate code for modals #1046

Merged
merged 8 commits into from
Jan 31, 2025

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jan 8, 2025

Introduces a generic modal component and replaces existing modal code with the new component. This intention here is to reduce code duplication and to make future generic changes to modals (i.e. focus trapping) easier.

How to test this

No special dependencies or configuration necessary, can be tested as is. Just click through every modal (i.e. "Event Details") you can find and check if it still properly opens and closes, and still looks like expected.

Introduces a generic modal component and
replaces existing modal code with the new
component. This intention here is to reduce
code duplication and to make future generic
changes to modals (i.e. focus trapping) easier.
@Arnei Arnei added the type:code-enhancement Internal improvements to the codebase label Jan 8, 2025
Copy link
Contributor

github-actions bot commented Jan 8, 2025

This pull request is deployed at test.admin-interface.opencast.org/1046/2025-01-23_15-56-31/ .
It might take a few minutes for it to become available.

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-1046

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-1046

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Arnei added 4 commits January 21, 2025 09:38
The introduction of the new modal component
broke the confirmation modal that appeared
when you had unsaved changes in your events
or series details access policy tab. Should now
be restored.
Arnei added a commit to Arnei/opencast-admin-interface that referenced this pull request Jan 21, 2025
Adds a confirmation modal that warns about unsaved changes
to the following tabs of the event details:
Metadata,
Extended Metadata,
Scheduling (for scheduled events),
Workflows (for scheduled events).
Thereby this fixes opencast#982.

Includes opencast#1046, because I was expecting this to be tightly related to
modals and did not want to write it twice. The general coding
approach would be the same though.

Speaking of approach, I would like feedback on if this makes
for a good approach, or if there are better alternatives I've
missed. If people agree with this approach it could then be
extended to other modals (e.g. series) as well.
Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@geichelberger
Copy link
Contributor

I found a bug. You can skip the stepper by clicking on the steps, at least for the events:

modal-bug.webm

@Arnei
Copy link
Member Author

Arnei commented Jan 31, 2025

I can reproduce the bug you showcased on the main branch, so this PR is probably not guilty of it. I'll create an issue for it.

Copy link
Contributor

@geichelberger geichelberger left a comment

Choose a reason for hiding this comment

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

LGTM, I clicked through a lot of modals, and they were all working as expected or broken like before :)

@geichelberger geichelberger merged commit adcdca0 into opencast:main Jan 31, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:code-enhancement Internal improvements to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants