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 modal content #1059

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jan 14, 2025

This code structure appears in like 50 files, so I'm putting it in a component so I don't have to see it again.

While there are many changed lines, 99% of them are indentation changes, so this is hopefully fairly easy to review.

How to test this

Can be tested as usual.

@Arnei Arnei added the type:code-enhancement Internal improvements to the codebase label Jan 14, 2025
Copy link
Contributor

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-1059

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-1059

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

github-actions bot commented Jan 14, 2025

This pull request is deployed at test.admin-interface.opencast.org/1059/2025-01-17_12-48-02/ .
It might take a few minutes for it to become available.

Copy link
Member

@ziegenberg ziegenberg left a comment

Choose a reason for hiding this comment

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

Could you separate the whitespace changes into a separate commit?

@Arnei
Copy link
Member Author

Arnei commented Jan 14, 2025

I can try.

@Arnei
Copy link
Member Author

Arnei commented Jan 14, 2025

Although the white space changes are directly related to the introduction of the new component, so it seems reasonable to me to have them in the same commit. Can you explain to me how separating them into their own commit helps with git blame?

@ziegenberg ziegenberg self-requested a review January 14, 2025 16:11
@ziegenberg ziegenberg dismissed their stale review January 14, 2025 16:12

If the white space changes are directly related to the introduction of the new component then leave them as-is.

Copy link
Contributor

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

This code structure appears in like 50 files, so
I'm putting it in a component so I don't have to
see it again.
@Arnei Arnei force-pushed the reduce-duplicate-code-modalcontent branch from 28b518a to 47d0018 Compare January 17, 2025 12:47
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:conflicts type:code-enhancement Internal improvements to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants