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

[FEATURE] ILIASObject: remove ilProgressBar usage #8270

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

Conversation

thibsy
Copy link
Contributor

@thibsy thibsy commented Oct 28, 2024

Hi @kergomard,

I have replaced the legacy ilProgressBar usage as fast and good as I could. I am not quite sure, if this code is actually reachable, because when copying multiple (nested) objects, the page simply loads without ever showing the ilObjectCopyProgressTableGUI table. Maybe you can try this out yourself?

Also note, I have replaced the legacy usage as is. I believe we should not render multiple progress bars on this page in the future, but instead use one progress bar to cover the progress of all objects being copied. I also needed to provide the _max_steps as an URL parameter, because there is (currently) no way to ping-pong the max-value of the progress bar.

Further note, this PR contains an additional commit which needs to be merged first. see #8268

Kind regards,
@thibsy

@thibsy thibsy added improvement php Pull requests that update Php code labels Oct 28, 2024
@thibsy thibsy force-pushed the feature/10/iliasobject-progress-bar branch from c2e0947 to 9655d2a Compare October 28, 2024 12:55
@thibsy thibsy force-pushed the feature/10/iliasobject-progress-bar branch from 9655d2a to d788c92 Compare October 28, 2024 13:23
Copy link
Contributor

@kergomard kergomard left a comment

Choose a reason for hiding this comment

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

Hi @thibsy

Thank you very much for the PR!

I finally got around to looking into this. I started by having a look at the functionality and sadly, this is not working as expected yet, so I would kindly ask you to have another look at it.

To be able to test it.

  • You need a category (or a course) with multiple objects in it.
  • You need to fix the soap-endpoint here (for me it was enough to just remove '/public' from the endpoint, ...but there might be more issues than that.

What isn't working (as far as I can see):

  • The progress bar is never updated, but disappears right away.
  • There should be a redirect once the copying has finished. We either never get there or it is not working.

I only read cursorily through the code and thus right now only have two minor, purely formal change requests:

  • Could you please use the standard comparison order here. From where I stand code becomes harder to read this way around.
  • Please move the usages of namespaces here with aliases (DataFactory, UIRenderer, UIFactory).

Thanks again and best,
@kergomard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants