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(forms): Adding dropdown category and related sub question type #17126

Merged

Conversation

ccailly
Copy link
Member

@ccailly ccailly commented May 15, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets

Added Dropdown category and related sub-question type : Dropdown

Form editor view :
image
image

Form end user view :
image

@ccailly ccailly self-assigned this May 15, 2024
@ccailly ccailly added this to the 11.0.0 milestone May 15, 2024
Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

Two small bugs I've found:

1) Wrong input name

image

Should be answers_141 since we switched to the new format.
This mean answers are not taken into account for this question type.

2) Invalid order

image

image

It seems the dropdown is not refreshed after options order are modified using drag and drop.

The "old" order is still displayed until we do another action like changing the default value.

src/Form/QuestionType/AbstractQuestionTypeSelectable.php Outdated Show resolved Hide resolved
src/Form/QuestionType/AbstractQuestionTypeSelectable.php Outdated Show resolved Hide resolved
src/Form/QuestionType/AbstractQuestionTypeSelectable.php Outdated Show resolved Hide resolved
src/Form/QuestionType/AbstractQuestionTypeSelectable.php Outdated Show resolved Hide resolved
@ccailly ccailly force-pushed the feature/add-dropdown-type-question-form branch from c5ced17 to bd6fc60 Compare May 22, 2024 09:55
src/Form/QuestionType/QuestionTypeDropdown.php Outdated Show resolved Hide resolved
js/form_question_dropdown.js Outdated Show resolved Hide resolved
js/form_question_dropdown.js Show resolved Hide resolved
src/Form/QuestionType/AbstractQuestionTypeSelectable.php Outdated Show resolved Hide resolved
templates/pages/admin/form/form_question.html.twig Outdated Show resolved Hide resolved
js/form_editor_controller.js Outdated Show resolved Hide resolved
@orthagh
Copy link
Contributor

orthagh commented May 22, 2024

(opinion based on the provided screenshots)
I feel the design choice when checking multiple flag to switch between radio or checkboxes for options lead to a bad understanding. At least it took a few seconds to understand the options will be displayed in a real dropdown. I almost made a comment about why call it dropdown.

I think in this particular case, you should not decorate options at all. More, there is the drag icon, so 2 icons on the left of the options

Ideas for alternative ux:

  • select2 tokenisation (may not work without multiple select)
  • modal with a [new option] link right to the dropdown
  • keep the same design (without decoration), but make options list like it's the dropdown opened (check this link)

@cedric-anne cedric-anne self-requested a review May 22, 2024 15:34
@cedric-anne cedric-anne marked this pull request as draft May 29, 2024 06:42
@ccailly
Copy link
Member Author

ccailly commented May 30, 2024

Capture d’écran du 2024-05-30 11-29-49

@cedric-anne cedric-anne marked this pull request as ready for review May 30, 2024 09:52
@ccailly ccailly force-pushed the feature/add-dropdown-type-question-form branch from c02da61 to 9e480e7 Compare June 5, 2024 13:04
@ccailly
Copy link
Member Author

ccailly commented Jun 11, 2024

image

@cedric-anne
Copy link
Member

@AdrienClairembault @orthagh
Is this OK for you now?

Copy link
Contributor

@AdrienClairembault AdrienClairembault left a comment

Choose a reason for hiding this comment

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

UI is good now.
Some changes requested for the code.

src/Glpi/Form/QuestionType/AbstractQuestionTypeActors.php Outdated Show resolved Hide resolved
@ccailly ccailly force-pushed the feature/add-dropdown-type-question-form branch from 1393d86 to 171489f Compare June 11, 2024 14:16
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

Seems globally OK, I just made a few minor remarks.

src/Glpi/Form/QuestionType/QuestionTypeDropdown.php Outdated Show resolved Hide resolved
src/Glpi/Form/QuestionType/QuestionTypeDropdown.php Outdated Show resolved Hide resolved
Comment on lines +263 to +265
// Check selected options and option labels
checkSelectedOptions([1]);
checkOptionLabels(["Option 1", "Option 2", "Option 3"]);
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker, but, IMHO, when we switch from multiple to single value dropdonw, we should do this:

  • if there are multiple values selected, all should be deselected,
  • if there is a unique value selected, it should remain selected.

Copy link
Contributor

@orthagh orthagh left a comment

Choose a reason for hiding this comment

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

As discussed together, the current design still has flaws (particularly choosing the default value dropdown, covering the rest of the design, leads to a strange state).
But they may be addressed later, especially with the right panel discussed many times.

Some I agree we're ok for this PR.

@ccailly ccailly force-pushed the feature/add-dropdown-type-question-form branch from 8022991 to 000b2be Compare July 1, 2024 12:52
@cedric-anne cedric-anne merged commit b04e0ac into glpi-project:main Jul 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants