Problem/Motivation
During the usability sessions, some participants struggled to figure out how to add multiple key|label values. Some didn't understand the relationship between the key and the label.
In good form design, we use the correct input types to provide guidance towards the expected input, a large freetext area is not intuitive and requires a lot of help text to understand.
Proposed resolution
Replace the text area with multiple inputs, automatically creating the value based on the machine name UI component.
Remaining tasks
Implement the suggestions
Design review
Test
User interface changes
Better widget than textarea for managing the list items. Something similar to https://www.drupal.org/project/options_element
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#54 | 2521800-11x.patch | 53.49 KB | bnjmnm |
#40 | Screen Shot 2023-06-13 at 1.52.29 PM.png | 252.02 KB | hooroomoo |
#40 | Screen Shot 2023-06-13 at 1.50.28 PM.png | 297.66 KB | hooroomoo |
Issue fork drupal-2521800
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
mErilainen CreditAttribution: mErilainen at Wunder commentedThis will most likely require an API change if done properly. Otherwise a transformation between an array and textarea has to be made like in the Options Elements module which I linked now to the issue description.
Comment #2
yched CreditAttribution: yched commentedIIRC @quicksketch had a contrib module which provided a UI pattern for those kind of needs.
Comment #5
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #6
dmsmidtThe proposal makes key|label entry much understandable than it is currently. Maybe we can even make it more clear by not mentioning 'key' at all, but just naming it 'value'. That said, I miss the 'Remove value' options per row in the image.
The descriptions says we add certain markup (is this still so?), I would say we shouldn't advertise it when the 'simple' widget is used.
So maybe we can also have an advanced widget (the current one) as well. Because bulk entry also seems tiresome with the 'simple' widget.
If we support both we would need a toggle for the two input modes.
Comment #7
tkoleary CreditAttribution: tkoleary at Acquia commentedWith select2 you can have a free text tag that transforms to a dismissable pill. Yet another reason to get that library into core.
Comment #8
dmsmidt@tkoleary that lib is similar to Chosen right?
Comment #9
tkoleary CreditAttribution: tkoleary at Acquia commentedRight, but a bit more robust and well adopted
Comment #20
lauriiiComment #22
lauriiiImplemented a prototype based on the proposed solution in the issue summary. The code definitely still needs clean-up, and it completely fails into taking into account other list field types.
There's probably still some minor modifications we should make to the UX. We should at least consider implementing a delete button for a row. This would improve the UX as without the delete button it's quite tedious to remove an option. We also could potentially disable the delete button when there's existing data since in this case options cannot be removed anymore.
Comment #24
tim.plunkettTweaked just enough to get tests to run, and then looked into the failures. Lots of "settings[allowed_values]" not found.
Also this could use #parents on the label/key fields to strip out all the table/item noise of those keys, but not doing that until we're more certain about the direction.
Comment #25
tim.plunkettComment #26
tim.plunkettWorking on this
Comment #27
Wim LeersThe same usability problem exists for:
\Drupal\ckeditor5\Plugin\CKEditor5Plugin\Style
\Drupal\ckeditor5\Plugin\CKEditor5Plugin\CodeBlock
— see #3282233-34: Ability to configure additional languages (e.g. "bash" or "SQL") for CKEditor 5 CodeBlock plugin.
P.S.: see
\Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::mapCKEditor4SettingsToCKEditor5Configuration()
and\Drupal\ckeditor5\Plugin\CKEditor5Plugin\Style::parseStylesFormValue()
for how we dealt with the transition from storing the configuration in this exact way to something more structured 🤓Comment #28
tim.plunkettI was not able to work on this more since my last commit.
Thanks for the pointers @Wim Leers!
Comment #31
srishtiiee CreditAttribution: srishtiiee at Acquia commentedComment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems to be several open threads on the MR
Comment #35
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #36
srishtiiee CreditAttribution: srishtiiee at Acquia commentedComment #37
Utkarsh_33 CreditAttribution: Utkarsh_33 at Acquia commentedComment #38
smustgrave CreditAttribution: smustgrave at Mobomo commentedThe screenshots in the issue summary appear to be outdated.
Did find it odd that when I added a field that it made 6 value fields by default. Shouldn't it just be 1 and allow users to add more as needed?
Also should the help text be moved to the top?
Referring to this
Wasn't super clear where or how the value is set, or that I could edit the machine name if I wanted to update the value of the option.
Comment #40
hooroomooUpdated issue summary with current screenshots
Comment #41
vasikei think this needs a "little review" ... at least to be checked what's left to be done ... if not "ready".
Comment #42
smustgrave CreditAttribution: smustgrave at Mobomo commentedMy points from #38 appear to have been addressed
Retested on 11.x
Added a list to the Article content type
Verified I see 1 field by default
Added a label and edited the machine name to something completely different
Added a second value
Created an Article
Inspecting the dropdown the labels and values match exactly what I did.
Also applied the patch back n forth and the list field updated without issue.
Comment #43
lauriiiDiscussed with @ckrina and she liked the improvement. However, she mentioned that it would be nice if we could display the float and integer fields side by side. Let's research if something that we could do easily because it seems like it would be a nice way to take this a step further 👍
Comment #45
narendraRAddressed #43 feedback.
Comment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed the change in #43. Excited to see this land!
Comment #47
srishtiiee CreditAttribution: srishtiiee at Acquia commentedComment #48
lauriiiComment #50
smustgrave CreditAttribution: smustgrave at Mobomo commentedRetested with List(text), Floats, ints and no new issues to report.
Comment #51
bnjmnmThis is looking good! I opened a few threads in the MR that should be addressed but it's probably all pretty simple stuff.
Comment #52
narendraRComment #53
smustgrave CreditAttribution: smustgrave at Mobomo commentedPoints appear to have been addressed.
Comment #54
bnjmnmThis MR was not very accommodating to a rebase so here's the 11x as a patch.
Comment #55
bnjmnmWas about to commit based on review/rtbc then noticed #43 which mentions
I'd like to at least clarify what that is asking for and see if it was in fact considered. I believe @lauriii will be around Monday to clear this up. Setting to postponed as it's possible this might not need additional work. If we decide to not pursue that change this can go back to RTBC.
Comment #56
bnjmnmLooking at the commit history I now see what was meant by the statement I wondered about in #55, and it has been completed satisfactorily.
Comment #58
bnjmnmI made some minor tweaks (its all in the history, and now this is committed to 11.x (which means it'll first appear in 10.2.
Really great improvements here, and its just ambitious enough that it is good for this to be at the beginning of a new release cycle.
Comment #60
quietone CreditAttribution: quietone at PreviousNext commentedComment #61
quietone CreditAttribution: quietone at PreviousNext commentedI meant to leave a comment.
This issue introduce a deprecation in \Drupal\options\Plugin\Field\FieldType\ListItemBase::extractAllowedValues and thus needs a change record. This is blocking #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard.
Comment #62
lauriiiCreated a CR: https://www.drupal.org/node/3376368
Comment #63
quietone CreditAttribution: quietone at PreviousNext commented@lauriii, Thank you!
Comment #64
catchThe changes to ListStringItem to use machine name break when there is existing data on a site that doesn't match the machine name format. Opened #3411419: Regression from #2521800: using machine name element for ListStringItem breaks with existing data to revert just that bit.
Comment #65
mglamanFor anyone arriving here, I've opened #3440019: ListStringItem is difficult to work with when you have a long pre-defined list