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

feat(Timeslot) Timeslot V1 with 2 ranges but no margin option #22

Merged
merged 30 commits into from
Feb 29, 2024

Conversation

Lainow
Copy link
Contributor

@Lainow Lainow commented Feb 1, 2024

image

@stonebuzz
Copy link
Contributor

@cconard96

I'd like your review of the JS part (it's not my area of expertise =) )

templates/package/timeslot.html.twig Outdated Show resolved Hide resolved
javascript/timeslot.js Show resolved Hide resolved
javascript/timeslot.js Outdated Show resolved Hide resolved
* -------------------------------------------------------------------------
*/

var AJAX_URL = CFG_GLPI.root_doc + '/' + GLPI_PLUGINS_PATH.deploy + '/ajax/timeslot.php';

Choose a reason for hiding this comment

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

The contents of this script should be encapsulated somehow to avoid adding/overwriting anything in the global scope. As far as I can tell, nothing outside this script calls any of the functions or uses any of the variables.

javascript/timeslot.js Outdated Show resolved Hide resolved
javascript/timeslot.js Outdated Show resolved Hide resolved
javascript/timeslot.js Show resolved Hide resolved
@stonebuzz
Copy link
Contributor

@Lainow

as a final step with a final review

We need to add glpi_plugin_deploy_timeslots_id column to table glpi_plugin_deploy_package_target.php

add update this view

image

to choose the timeslot when selecting the computer group

and display it in the table below

@stonebuzz
Copy link
Contributor

@Lainow

waiting for the CI Continuous integration to be adapted to GLPI 11.0.x

@stonebuzz stonebuzz merged commit 5909326 into pluginsGLPI:main Feb 29, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants