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

Make next/previous buttons a bit more robust #856

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

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jul 30, 2024

Makes the nextPage/previousPage functions for the create event/series modals a bit more robust, by removing assumptions about how many hidden pages in a row are specified in the steps array.

Makes the nextPage/previousPage functions for
the create event/series modals a bit more robust,
by removing assumptions about how many hidden
pages in a row are specified in the steps array.
@Arnei Arnei added the type:code-enhancement Internal improvements to the codebase label Jul 30, 2024
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-856

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

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 Jul 30, 2024

This pull request is deployed at test.admin-interface.opencast.org/856/2024-08-01_12-52-09/ .
It might take a few minutes for it to become available.

let newPage = page;
do {
newPage = newPage + 1;
} while(steps[newPage] && steps[newPage]!.hidden);
Copy link
Member

@lkiesow lkiesow Jul 30, 2024

Choose a reason for hiding this comment

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

This is more out of curiosity and the fact that I didn't know about the null assertion operator !. until now. What is the reasoning behind using it here? From what I read, the operator will add a runtime check ensuring (it would throw an error) that steps[newPage] is not null. If the assertion fails, I think this would break the UI. That is why you have the check that it can never be null as first part of the condition. But then… you don't need the assertion because it is already always guaranteed to pass, isn't it? I'm somewhat struggling to see where it would make sense to use this operator and don't know why it would make sense here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah you're right, the ! operator is a left-over that is not needed here anymore. I've removed it.

The null assertion operator is to be used in cases where you successfully determined that your variable is not null | undefined, but typescript fails to infer on that. You can then use ! to shut typescript up.
But more often than not using ! points to a problem in your code that you should fix instead of using !.

Also remove unused parameter
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