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

Extract manuallink form to a controller #18486

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Pierstoval
Copy link
Collaborator

No description provided.

Copy link
Contributor

@trasher trasher left a comment

Choose a reason for hiding this comment

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

Seems good but I do not know how to test it, I cannot reach front/manualink.form.php on current master (maybe a permission issue, ,no idea).

@cconard96
Copy link
Contributor

Seems good but I do not know how to test it, I cannot reach front/manualink.form.php on current master (maybe a permission issue, ,no idea).

The itemtype and items_id params are required in the URL currently. The form probably should have been inline with the link list for the asset instead of a separate page.

@Pierstoval
Copy link
Collaborator Author

The form probably should have been inline with the link list for the asset instead of a separate page.

Separate pages seem to be used for all instances of CommonDBChild, I guess the reason is that most forms are loaded via AJAX and it's easier this way.

@cconard96
Copy link
Contributor

cconard96 commented Dec 6, 2024

The form probably should have been inline with the link list for the asset instead of a separate page.

Separate pages seem to be used for all instances of CommonDBChild, I guess the reason is that most forms are loaded via AJAX and it's easier this way.

Some things like rule actions and criteria are embedded via a viewsubitem.php AJAX call. Their .form.php files only exist to handle add/update/delete but not show. One of my recent PRs even adds a Twig template for viewsubitem that optionally submits the form over AJAX to avoid page reloads.

@Pierstoval
Copy link
Collaborator Author

@cconard96 what would you recommend then?

@cconard96
Copy link
Contributor

My suggestion since it is a very small form is to display it inside the Link tab for assets.

When the tab loads, the display remains as it is now with the 2 buttons and then a list of links under them.
When clicking the "Add" button, the button disappears the manual link form is displayed using the viewsubitem template.
When clicking a row in the list that corresponds to a manual link, the viewsubitem form is shows with the data for that link.
When clicking a row for a regular link, it redirects to the Link form.

I'm not saying it has to be done here and I can make this change myself in another PR if other people agree with this UX.

@cedric-anne cedric-anne self-assigned this Jan 13, 2025
@cedric-anne cedric-anne marked this pull request as draft January 13, 2025 10:44
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.

4 participants