-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[11.0] Add a "progress" system #18296
Conversation
a37ee65
to
1894444
Compare
Latest changes' version: inst.mp4 |
21599e8
to
2441414
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, to have a truly "generic" component we need to simplify the js implementation.
In this install example you have to create a specific glpi_install.js
file with 150 lines of code.
It would be much easier to re-use the component if it required no JS at all for new implementations.
For example, we could watch for a specific data attribute that would turn any form into a progress bar when submitted:
<form
data-glpi-progress-endpoint='/Install'
data-glpi-progress-on-complete='my_optional_js_callback()'
>
...
</form>
Then we watch globally for submit events on form[data-glpi-progress-endpoint]
, requiring no further js code for the feature itself.
We can add more data attribute like data-glpi-progress-on-complete
if there is a need for custom hooks for some features (I suppose most would need a redirect at the end ?).
On the backend side, the check_progress
action should probably not be specific to the install controller too, as all progress bar will require this action.
We discussed about it on the internal chat. The progress checking could indeed be generic. It would be something like a dedicated JS module that can be instanciated with a |
I moved the code a bit, I'm doing it step by step to put the most possible amount of code outside the The tricky part for now is moving everything related to the DOM outside, but I'm on it, the function will just need a DOM element as argument and the error, callbacks and other params |
Here, I think I made a good "final code". Here's how it works:
Video
|
46604f8
to
e128473
Compare
bda07d6
to
016cd0f
Compare
@@ -58,6 +58,7 @@ | |||
$services->load('Glpi\Controller\\', $projectDir . '/src/Glpi/Controller'); | |||
$services->load('Glpi\Http\\', $projectDir . '/src/Glpi/Http'); | |||
$services->load('Glpi\DependencyInjection\\', $projectDir . '/src/Glpi/DependencyInjection'); | |||
$services->load('Glpi\Progress\\', $projectDir . '/src/Glpi/Progress'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is is alright to load the full Progress
directory here ?
SessionProgress
is not a service, shouldn't loading it in the dependency injection trigger an error as its constructor expect some parameters ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this whole file should be avoided.
3rd party UI components like bootstrap's dropdown or modals are configured only using html's data attribute and IMO we should be able to do the same here as it make them much more convenient to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a back
and a continue
button that have to be displayed at the end of the process. This will probably be different in other usages of this component. Also, the endpoint to fetch may require some parameters, and it may be difficult to pass them via data-attributes. For instance, for the massive actions, we will want to send data in JSON format to remove the current limitation related to the form format (max_input_vars
limitation).
Maybe the error message display could be mutualize, we will see when we will use this component in other contexts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a back and a continue button that have to be displayed at the end of the process.
The buttons are already identified by some unique identifier like glpi_install_success
.
It wouldn't be too hard to change it to something like data-glpi-progress-show-on-success
.
This will probably be different in other usages of this component.
I agree that there may be rare edge cases where it will be needed to have some custom code.
But we should still be able to provide something simple to use for the common cases (I suppose most progress bar will either display something or redirect the user once they are finished) through data attributes.
Taking the boostrap example once again, they have data attribute for common cases and emit some events so that specific behavior can be implemented by adding listeners.
For instance, for the massive actions, we will want to send data in JSON format to remove the current limitation related to the form format (max_input_vars limitation)
I think this should be handled by using the appropriate HTML elements.
For example, the install progress display a <progress data-glpi-progess='...'></progress>
item that is executed when the page is displayed.
The massive actions would instead use a <form data-glpi-progress='...'></form>
item instead that is executed when the form is submitted (thus passing the JSON along).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against mutualizing code. It is something we could try to achieve when we will use the progress bar system in massive actions.
Goal is to create an abstraction for a "progress bar" system.
For now, it works based on session, but the service is standalone enough to work on other storages later.
First usage is on the installation UI, here is how it looks like for now (no warranty on visuals, they may change later, of course):
inst.mp4