Problem/Motivation

#2349991: Provide a trait for categorizing plugin managers and use it for conditions and actions will introduce a generic concept of "categorized plugins". it would be nice to implement it for field types in order to improve the UX of the field type selection in Field UI.

Proposed resolution

Do it?

Remaining tasks

Decide on the category names. Latest proposal is in comment #15.

User interface changes

Field types will be grouped by "category" in the field type selection when adding a new field.

API changes

Nope.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Prioritized changes The main goal of this issue is to improve usability in Field UI.
CommentFileSizeAuthor
#30 interdiff.txt542 bytesamateescu
#30 2373491-30.patch16.65 KBamateescu
#17 interdiff.txt2.21 KBamateescu
#17 2373491-17.patch16.64 KBamateescu
#15 interdiff.txt2.09 KBamateescu
#15 2373491-15.patch14.43 KBamateescu
#10 interdiff.txt7.67 KBamateescu
#10 2373491-10.patch14.16 KBamateescu
#8 2373491-8.patch15.84 KBjibran
#8 interdiff.txt2.77 KBjibran
#3 Screenshot from 2015-02-02 01:10:38.png35.43 KBjibran
#3 2373491-3.patch12.48 KBjibran
#1 categorize-field-types.patch12.4 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Active » Postponed
FileSize
categorize-field-types.patch12.4 KB

We need to wait for #2349991: Provide a trait for categorizing plugin managers and use it for conditions and actions to land first but here's something to get this started.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Postponed » Needs review

The blocking issue was committed some time ago, we can start bikeshedding the field type group names now :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2373491-3.patch12.48 KB
Screenshot from 2015-02-02 01:10:38.png35.43 KB

Here is quick reroll and screenshot.

The last submitted patch, 1: categorize-field-types.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2373491-3.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Term reference should be a reference?

amateescu’s picture

amateescu CreditAttribution: amateescu commented

I didn't include it on purpose because I prefer to think we'll get #1847596: Remove Taxonomy term reference field in favor of Entity reference in at some point :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
Issue tags: +Need issue summary update
FileSize
interdiff.txt2.77 KB
2373491-8.patch15.84 KB

Fixed test. We need BE in IS.

yched’s picture

yched CreditAttribution: yched commented

For easy review, can we extract a list of groups / fields as plain text ?

For example, I'm not sure about the consistency of the current proposed group names (names like "References" vs adjectives like "Numeric")

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2373491-10.patch14.16 KB
interdiff.txt7.67 KB

I don't think the interdiff from #8 is correct, we should not modify valid tests just because the current implementation of FTPM::getUiDefinitions() is wrong. Updated the patch a bit, interdiff is to #3.

This is the current group naming:

Boolean
Comments
Date
Email
Link
Number
  List (float)
  List (integer)
  Number (decimal)
  Number (float)
  Number (integer)
  Telephone number
Reference
  Entity reference
  File
  Image
  Term Reference
Text
  Text (plain)
  Text (formatted, long)
  List (text)
  Text (formatted)
  Text (formatted, long, with summary)
  Text (plain, long)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Can we add

Boolean
Comments
Date
Email
Link

to others category i.e. add all the fields without category to others/misc group?

amateescu’s picture

amateescu CreditAttribution: amateescu commented

I was considering the same thing but "other/misc" would imply that those field type are of a lesser importance than the other ones, and we certainly don't want to transmit that message.

Other suggestions for a generic group name are appreciated :)

jclema’s picture

jclema CreditAttribution: jclema commented
Issue tags: -Need issue summary update +Needs issue summary update
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue summary: View changes
Issue tags: -Needs issue summary update +Needs usability review

Let's try the bat signal.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2373491-15.patch14.43 KB
interdiff.txt2.09 KB

Other suggestions for a generic group name are appreciated :)

In fact, 'generic' is exactly what we're looking for here. I researched a bit on 'generic' vs. 'general' and it seems that the latter usually has a positive connotation, so let's go with 'General'.

This is the current UI grouping:

General
  Boolean
  Comments
  Date
  Email
  Link
Number
  List (float)
  List (integer)
  Number (decimal)
  Number (float)
  Number (integer)
  Telephone number
Reference
  Entity reference
  File
  Image
  Term Reference
Text
  Text (plain)
  Text (formatted, long)
  List (text)
  Text (formatted)
  Text (formatted, long, with summary)
  Text (plain, long)

Status: Needs review » Needs work

The last submitted patch, 15: 2373491-15.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs work » Needs review
FileSize
2373491-17.patch16.64 KB
interdiff.txt2.21 KB

I love that test.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Needs issue summary update

+1 for General. I think we need beta evaluation in issue summary.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue summary: View changes
Issue tags: -Needs issue summary update

Done.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue summary: View changes
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

This is RTBC for me but let's wait for batmen(usability review) and spiderman(@yched) :D

yched’s picture

yched CreditAttribution: yched commented

Spiderman : lol - unfortunately I'm not exactly that speedy atm. But the suit is cool ;-)

This looks great IMO. Leaving at NR for usability review.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » Bojhan

The grouping makes sense to me as well. Explicitly assigning to Bojhan for review, since he was involved in the parent issue. If no response in a couple of days, I think we can go with this and tweak later.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Assigned: Bojhan » Unassigned
Issue tags: -Needs usability review

Looks good to me, its a bit odd to see Text at the bottom.

Should Link not be in reference? Either way a big step already.

@webchick Thanks for time-boxing, I don't want to be a bottleneck. I also have no idea how to actually keep taps on this, other than checking my queue every day - which I am finding harder and harder.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Thanks for taking a look!

'Text' is at the bottom because the list is ordered alphabetically, do you think we should hardcode it to a different position?

I'm not sure about link.. I guess you could think of it as a "URL reference", how do others feel about that?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

If we put "Link" in the "Reference" category (because it's a reference to a URL), then I think we also need to put "Email" there, and arguably even "Date".

IMO, "Link" is fine where it is.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community

Asked Bojhan what he thought about #26 & #27:

WimLeers: Bojhan: see my comment of one minute ago — what do you think?
Bojhan: WimLeers: dont feel strongly about it
WimLeers: ok, then I'm going to RTBC it
Bojhan: WimLeers: RTBC that shit

:D

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Nitpick that can be fixed upon commit:

+++ b/core/lib/Drupal/Core/Field/Annotation/FieldType.php
@@ -54,6 +54,15 @@ class FieldType extends DataType {
+   * The category under which the condition should listed in the UI.

s/should/should be/

amateescu’s picture

amateescu CreditAttribution: amateescu commented
FileSize
2373491-30.patch16.65 KB
interdiff.txt542 bytes

A better nitpick would be to realize that the category is for a field type, not a condition :P

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

LOL!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6ba9484 on 
    Issue #2373491 by amateescu, jibran: Categorize field type plugins

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.