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

Revert PR #1043 "Update react-router to 7.1.1" #1083

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jan 23, 2025

Reverts PR #1043, specifically commit a4097a5. The PR seems to have caused problems with table filtering, which are fixed by reverting it. The issue in question is that clicking on a series name in the series table will not result in a properly filtered events table anymore. See also #1043 (comment)

How to test this

Can be tested as usual. Make sure to run npm install.

Reverts PR opencast#1043. The PR seems to have caused
problems with table filtering, which are fixed by
reverting it. The issue in question is that clicking
on a series name in the series table will not result
in a properly filtered events table anymore.
@Arnei Arnei added type:bug Something isn't working type:dependencies Pull requests that update a dependency file labels Jan 23, 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-1083

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

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 23, 2025

This pull request is deployed at test.admin-interface.opencast.org/1083/2025-02-11_09-58-48/ .
It might take a few minutes for it to become available.

@Arnei Arnei changed the title Revert a4097a5f84c8d67d9774a55f4c73247d900e36d2 Revert PR #1043 "Update react-router to 7.1.1" Jan 28, 2025
Copy link
Contributor

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

1 similar comment
Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Feb 4, 2025

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

@owi92
Copy link
Contributor

owi92 commented Feb 7, 2025

This doesn't seem to be working for me. The filter only gets applied sometimes, and when it does, it still takes a few seconds to display the right events.

Bildschirmaufnahme.2025-02-07.um.16.05.56.mov

And just as a side note for anyone else testing this branch, I had to take some extra steps before I could do so:
- before running npm install, I had to run rm -rf node_modules package-lock.json. Without doing that first, the app wouldn't start, because it Cannot find module @rollup/rollup-darwin-arm64. This seems to be related to npm/cli#4828.
- After doing rm -rf node_modules package-lock.json followed by another round of npm install, the app still didn't start correctly, informing me in devtools that there is an Uncaught Error: Could not resolve "@emotion/styled" imported by "@mui/styled-engine". Is it installed?
- so I also had to run npm install @emotion/styled @emotion/react
- with that, the app will start

@Arnei
Copy link
Member Author

Arnei commented Feb 10, 2025

Try as I might, I cannot reproduce your flukes. Is there any additional information to be had (in the webconsole etc.)?

Filters not being applied instantly is arguably a different issue.

@owi92
Copy link
Contributor

owi92 commented Feb 10, 2025

I just tried again, with the same outcome. There are no failed requests or errors in the console. I am on Node.js v20.18.2, if that is of any importance.
It's probably best if another person tests this, and if they don't have any issues, I think we can ignore mine. Just reverting that commit theoretically shouldn't break anything that wasn't broken before.

Also agree that filters not being applied instantly is another issue, it's just something that becomes apparent when testing these.

Copy link
Contributor

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

@Arnei Arnei force-pushed the revert-react-router-7.1.1 branch from 8248cc1 to 673b26e Compare February 11, 2025 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working type:dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants