Problem/Motivation

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes

Issue category Task (not a feature) because the form already exists, and the patch does not add or remove any functionality.
Issue priority Major because the Field UI is one of the most important site builder UIs, and it consistently trips users up in usability studies. This change also will make further UX improvements to the Field UI more feasible. It is not critical because the current UI already exists in Drupal 7.
Prioritized changes The main goal is to improve both the usability and accessibility of this important form, so it is a prioritized change for the beta phase.
Disruption
  • This change will require rework to existing modules that alter the manage fields form.
  • Many tests will need to be updated because they include assumptions about the current UI.
  • The disruption to core will be limited to the Field UI itself.

The impact of this change is clearly greater than the disruption, so this is a good issue to complete during the beta.(@webchick in #52, @xjm in #167.)

Proposed resolution

  • Remove the "Add new field" and "Reuse existing field" rows from the manage fields table.
  • Add a local action button for "Add field".
  • "Add field" provides a form where the user:
    1. Enters the field label, which automatically generates a machine name as per usual.
    2. Selects either "Create new field" or "Reuse existing field", which reveal options with states...
      • For "Reuse existing field," the option reveals a selection box with the existing fields.
      • For "Add new field," the option reveals the field type selection box as well as the field settings (like maximum # values, entity reference entity type, etc.)
      • For both, the widget selection element is shown.
  • For now, submitting this form takes the user to the existing field instance settings form.

Test steps

  1. Go to content types -> basic page -> Manage fields
  2. Add a field

UI changes

New button on manage fields

New add field screen

Error message when the field exists

CommentFileSizeAuthor
#228 Add-Field-Error-en.png53.5 KBxjm
#228 Add-Field-en.png34.78 KBxjm
#228 Manage-Field-en.png55.73 KBxjm
#222 interdiff.txt1.07 KBamateescu
#222 1963340-222.patch95.49 KBamateescu
#220 interdiff.txt13.92 KBamateescu
#220 1963340-220.patch95.5 KBamateescu
#217 Screenshot from 2014-12-14 00:02:32.png172.45 KBswentel
#213 Add-Field-Error-ur.png69.57 KBjibran
#213 Add-Field-ur.png57.45 KBjibran
#213 Manage-Field-ur.png86.69 KBjibran
#213 Manage-Field-en.png94.53 KBjibran
#213 Add-Field-Error-en.png68.61 KBjibran
#213 Add-Field-en.png52.86 KBjibran
#210 1963340-210.patch95.66 KBamateescu
#209 interdiff.txt9.01 KBamateescu
#209 1963340-209.patch105.24 KBamateescu
#208 interdiff.txt39.07 KBamateescu
#208 1963340-208.patch94.98 KBamateescu
#202 interdiff.txt5.51 KBamateescu
#202 1963340-202.patch90.59 KBamateescu
#201 interdiff.txt20.62 KBamateescu
#201 1963340-201.patch90.23 KBamateescu
#188 add-field-5.png22.08 KByoroy
#187 add-field-4.png31.59 KByoroy
#187 add-field-3.png70.74 KByoroy
#187 add-field-2.png46.45 KByoroy
#187 add-field-1.png29.6 KByoroy
#187 add-field-0.png41.49 KByoroy
#186 interdiff.txt5.61 KBamateescu
#186 1963340-186.patch87 KBamateescu
#181 interdiff.txt8.13 KBamateescu
#181 1963340-181.patch94.91 KBamateescu
#171 Field UI 2.png25.45 KByched
#171 Fiield UI 1.png26.39 KByched
#170 interdiff.txt35.41 KBamateescu
#170 1963340-170.patch94.18 KBamateescu
#162 interdiff.txt2.75 KBamateescu
#162 1963340-162-do-not-test.patch66.37 KBamateescu
#161 Add_field___Site-Install.png468.62 KBLewisNyman
#159 1963340-159.patch66.41 KBamateescu
#157 Screen Shot 2014-11-05 at 5.03.58 PM.png159.37 KBmgifford
#156 1963340-156-do-not-test.patch66.41 KBamateescu
#155 1963340-155-do-not-test.patch66.46 KBamateescu
#143 interdiff.txt10.65 KBamateescu
#143 1963340-143-do-not-test.patch91.94 KBamateescu
#142 interdiff.txt3.38 KBamateescu
#142 1963340-142-do-not-test.patch91.92 KBamateescu
#138 drupal_1963340_138.patch84.23 KBXano
#137 1963340-137-do-not-test.patch91.71 KBamateescu
#136 interdiff.txt1.1 KBamateescu
#136 1963340-136-do-not-test.patch90.52 KBamateescu
#132 interdiff.txt7.64 KBamateescu
#132 1963340-132-do-not-test.patch90.56 KBamateescu
#130 1963340-130-do-not-test.patch90.38 KBamateescu
#129 1963340-129-do-not-test.patch88.55 KBamateescu
#127 interdiff.txt786 bytesamateescu
#127 1963340-127.patch0 bytesamateescu
#125 interdiff.txt11.51 KBamateescu
#125 1963340-rewritten-125.patch88.62 KBamateescu
#120 interdiff.txt74.29 KBamateescu
#120 1963340-rewritten.patch89.46 KBamateescu
#114 Screen Shot 2013-10-09 at 11.34.33.jpg211.39 KBLewisNyman
#114 Screen Shot 2013-10-09 at 11.37.24.jpg150.99 KBLewisNyman
#114 Screen Shot 2013-10-09 at 11.46.29.jpg160.93 KBLewisNyman
#112 field_ui-add.png17.67 KBandypost
#112 interdiff.txt2.34 KBandypost
#112 drupal8.field-ui.module.1963340-112.patch59.6 KBandypost
#111 interdiff.txt19.63 KBandypost
#111 drupal8.field-ui.module.1963340-111.patch59.46 KBandypost
#109 drupal8.field-ui.module.1963340-109.patch47.81 KBandypost
#105 interdiff.txt1.31 KBandypost
#105 drupal8.field-ui.module.1963340-105.patch47.8 KBandypost
#103 drupal8.field-ui.module.1963340-103.patch46.49 KBandypost
#103 interdiff.txt1.86 KBandypost
#98 interdiff.1963340.95.98.txt875 bytespguillard
#98 field_ui-1963340-98.patch46.4 KBpguillard
#97 add-field.png94.12 KBrteijeiro
#95 field_ui-1963340-95.patch46.45 KBpguillard
#95 interdiff.1963340.89.95.txt801 bytespguillard
#92 interdiff89-92.txt1.5 KBpguillard
#92 field_ui-1963340-92.patch46.86 KBpguillard
#90 AddFieldButton.png71.21 KBmgifford
#90 AddFieldScreen.png62.18 KBmgifford
#90 EditField.png86.62 KBmgifford
#90 SaveField.png61.32 KBmgifford
#89 interdiff.txt1.38 KBHydra
#89 field_ui-1963340-89.patch45.67 KBHydra
#87 field_ui-1963340-87.patch45.63 KBHydra
#85 field_ui-1963340-85.patch44.6 KBswentel
#85 interdiff.txt6.56 KBswentel
#84 field_ui-1963340-83.patch45.2 KBagentrickard
#84 interdiff-79-83.txt1.56 KBagentrickard
#79 field_ui-1963340-79.patch45.24 KBagentrickard
#79 interdiff-1963340-78-79.txt7.55 KBagentrickard
#78 field_ui-1963340-78.patch44.06 KBagentrickard
#76 contact-entity-type.png221.62 KBYesCT
#76 field_ui-1963340-76.patch49.02 KBYesCT
#76 interdiff-74-76.txt1.87 KBYesCT
#74 field_ui-1963340-ux-reroll-74.patch48.82 KBYesCT
#74 interdiff-71-74.txt5.37 KBYesCT
#71 field_ui-1963340-ux-reroll-71.patch38.49 KBagentrickard
#71 interdiff-1963340-66-71.txt14.49 KBagentrickard
#66 field_ui-1963340-ux-reroll-66.patch25.69 KBagentrickard
#63 field_ui-1963340-ux-reroll-63.patch31.4 KBagentrickard
#56 field_ui-add-field-separate-task-1963340-56.patch35.35 KBdags
#49 field-1963340-48-do-not-test.patch59.3 KBtim.plunkett
#47 field_ui-add-field-separate-task-1963340-47.patch48.55 KBdags
#47 interdiff.txt23.37 KBdags
#46 field_ui-add-field-step-1.png29.75 KBdags
#46 field_ui-add-field-step-2.1.png54.44 KBdags
#46 field_ui-add-field-step-2.2.png42.03 KBdags
#46 field_ui-add-field-step-3.1.png50.87 KBdags
#46 field_ui-add-field-step-3.2.png64.91 KBdags
#44 field_ui-add-field-separate-task-1963340-44.patch26.19 KBdags
#44 interdiff.patch23 KBdags
#41 field_ui-add-field-separate-task-1963340-41.patch28.98 KBmgifford
#37 field_ui-add-field-separate-task-1963340-36.patch8.89 KBmgifford
#30 field_ui-add-field-separate-task-1963340-30.patch28.99 KBdags
#23 field-ui-idea.png66.34 KBainz
#1 manage_fields_redesign.png36.53 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

FileSize
36.53 KB

No idea what is going on with attachments to issues this week. This is a mockup of the manage fields screen.

manage_fields_redesign.png

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

The only feature that is lost, is the ability to add a field in the correct order while creating it. This becomes especially nice when using field groups.
Not a reason to not do this at all, just identifying an interaction pattern that will go away.

xjm’s picture

@dawehner also suggested this could go in a modal. Possibly a separate issue as we'd want to do that for lots of local actions, I think.

jibran’s picture

So it will be like adding rules action in D7.

amateescu’s picture

Re #3: This was discussed in another issue and Bojhan was opposed to the idea: #1855992-8: Use dialog api for formatter settings.

yched’s picture

The only feature that is lost, is the ability to add a field in the correct order while creating it. This becomes especially nice when using field groups.

True, and I did find that feature was hot when we added it in CCK D6 :-). "Add a new field straight at the right position and fieldgroup", instead of "create it, then come back at the 'Manage fields' screen and drag it to the right place"...

But given the restrictions it imposes on the "field creation" UI (basically, all of the required info about the field you want to create , most notably the field type, need to fit in one single table row), I'm starting to think that we might want to reconsider whether this brings a usability win or loss.

But right, that's what's in the balance :-)

xjm’s picture

Priority: Normal » Major

Upon reflection, I think this is a major.

yched’s picture

Also, I probably should clarify that I'll be more than busy with "Field API / CMI" + its followups, then "Field types as plugins" and "Unify entity fields and field API", and thus cannot really plan on leading Field UI reshuffles myself :-/ (other than chiming in if needed, of course).

Bojhan’s picture

I would support this change :) It will be a small gain, but a considerable one when you have loads of distracting elements on the page

dags’s picture

Assigned: Unassigned » dags

xjm coaxed me into this one. ;) ...working on an initial patch.

falcon03’s picture

I strongly agree that adding a new/sharing an existing field should be separate tasks. And I would like to see those separate forms appearing in modals! :-)

As @xjm said, the current field UI to add a new/share an existing field has some accessibility issues. I created an issue at:
#1829368: A blind user cannot understand which form fields are required to add a new field or share an existing one
in November 2012. But this issue will fix it automatically, so I am closing it as a duplicate...

swentel’s picture

Bojhan was against modals for this one, as you don't use modals for a wizard (and also potentially large screens depending on the field type).

falcon03’s picture

@swentel: thanks for clarifying. I would like having them in separate forms as well: it's always better than the current UI, IMO.

dags’s picture

swentel’s picture

Also over at #1426804: Allow field storages to be persisted when they have no fields. we have another use case for what would have been a 3rd row which allows you to select a field that has no instance yet - having that in the 'Add field' selection form now would make it easier, unless we want to scare even more people ;-)

swentel’s picture

Issue tags: +Field API

Tagging also

ainz’s picture

Just a thought. On the form that the user is taken to when they click the "Add Field" button, what about adding a weighted dropdown list populated by the number of fields in the content type. This way the user can select the weight of the field being added in relation to the fields that are already in the content type (like the menu form). It's not as good as the drag and drop functionality currently but it would help. The user would have to know the number they want to set the field to before they click the "Add Field" button though.

tim.plunkett’s picture

Those weighted numbers are only useful if you can still see the old ones...
I'm not saying it's a huge problem to lose that ability, just that we should make a conscious decision to make this switch.

klonos’s picture

Personally I wouldn't mind if we decided to stick to the old ways, but I've been using D6, D7 and I'm kinda used to that. I've been following the progress of D8 development and so I mostly know what's changed, where and in what way. So, I'm also fine if we change things to what is discussed here.

I think that this separation is good for newcomers that jump straight to D8 though because it provides a more consistent and clean UI. First add fields in a separate, modal form, then come back to the list of fields to re-arrange. Simple!

dags’s picture

I started working on this new form yesterday and had a couple ideas I thought I'd throw out there.

I remember first starting out with Drupal and being confused by the Manage fields form. I didn't really understand that I didn't have to create a new Address field if one already existed on another content type, so I'd end up with fields like 'field_person_address' and 'field_place_address'. It might be less confusing if the first step of the field-adding-process was to ask the user what kind of field they want to add and THEN - if a field of that type already exists - have something like, "a field of this type already exists... would you like to reuse one of these fields..."

Also, what if we create the Add field button as it looks in xjm's mockup and when it's clicked, a new row is inserted via AJAX to the bottom of the field list?

klonos’s picture

I like the idea of using AJAX to insert a new row to the table because it would keep people in context. But...

For UI consistency across all admin forms, I'd like to leave the "Add field" button where it is in the mockup in #1 (top-left corner). OTOH I expect new fields added via AJAX at the bottom of the table to be immediately shown. That won't be the case though if there are enough fields to have the table span bellow the bottom of the screen. Unless we also auto-scroll at this newly crated row.

I'd like it less it if we placed the "Add field" button at the bottom-right corner as a "add another" button, because it would be less prominent (and cause UI inconsistency), but that would make the insertion of the row feel more natural and it would be within view.

I don't know what to choose :/

effulgentsia’s picture

I didn't really understand that I didn't have to create a new Address field if one already existed on another content type, so I'd end up with fields like 'field_person_address' and 'field_place_address'.

I leave it to others here to figure out what the scope of this issue should be. Given how late we are in the D8 cycle right now, I suggest keeping it reigned in as tight as possible though. But with respect to when you want a new field vs. when you want to reuse an existing field, it's ultimately almost entirely about what will make sense in Views. In the example above, is your site content model such that "person address" and "place address" are different kinds of data that you'll want Views treating as different fields? Or do you want Views treating it as the same "address" field and be able to return a set of nodes having that field, where some of the nodes can be "person" nodes and others can be "place" nodes? Like I said, I don't know that it should be this issue's responsibility to convey this information to the user, but I hope this comment is helpful.

ainz’s picture

FileSize
66.34 KB

I think that the order while creating is a great feature and I also think that the add field should be separated from the list of fields already there. As mentioned before when the list grows this can make adding a field less visible and the user would have to scroll.

Again, I'm just playing with this idea but what if we offer both on the same page but have the add field buttons to the top where they would remain? I actually think this is better than adding a button right now, but that's just me. I attached this mockup below to help explain what I mean. With a design like this users can still order the fields using the weights and they would be able to see what weight to put in because the list of fields currently in the content type would be below.

Also, maybe this might be easier than changing the design completely, since we're already late in the D8 cycle.

Field UI idea

klonos’s picture

I've never been a big fun of "forcing" the user to handle placement using row weights. Mainly because I don't think that everyone grasps the idea from the start (unless of course they are long-time Drupal users). Drag-n-drop WYSIWYG placement feels like much better UX.

Splitting the creation of rows to a separate table instead of a modal does have the advantage of keeping the user in context while keeping these two "special" rows separated from the existing fields. I like that. Having that at the top of the form also provides a natural top to bottom navigation feeling of first creating, then placing.

So, if we went that way, how about we added a "Add field" or "Create field" button in the actions column for both the new and the re-use existing field rows? Hitting that button would create the respective type of field as a new row at the top of the fields in the second table (in an AJAXy way). That row would have a different, yellowish color and the usual asterisk to denote that it's placement is not final nor saved. The user could then use the drag handlers to re-arrange and when happy finally, hit the "Save" button.

So this sound appealing all in all, but if we're not too late, I'd like to throw another, daring idea on the table:... Scratch that! I was about to propose to re-use the place block wizard we've now got in place, but I had a terrible experience using it :/

ainz’s picture

@klonos I like the idea that you proposed with the "Add Field" button but then this would mean that the user would have to click to save the field in the table below and then click again on "Edit" to add any configurations to that field.
Currently as it is when a user adds a field and they click save they automatically go to the configuration page for that field. Although the idea you proposed works great for reordering the field it adds another step for configuration. New users may not know they have to configure the field unless they are carried to that configuration screen automatically.

tim.plunkett’s picture

dags’s picture

Still working on this but my time has been a bit limited lately. I should be able to get a patch in by the end of this week or next.

dags’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

@dags, it'd be better to post whatever you have since the Drop is moving pretty quickly these days. :) Otherwise you're going to probably run into issues merging HEAD!

swentel’s picture

Also note that #2014821: Introduce form modes UI and their configuration entity is in the queue and will affect field ui quite heavily as well - note that I consider that one way more important to go in first.

dags’s picture

Status: Needs work » Active
FileSize
28.99 KB

Ok here's what I've got.

I've added a new inline-action button called 'Add field' to the FieldOverview form and converted the add-new an reuse-existing form elements to work outside of their former table-based roots. The biggest difference is that the fields use the #states API to determine whether to show the 'add new' or 'reuse existing' field settings. This required hacking field_ui.js a bit to get the field widgets options updating because as it stands right now, that file expects those fields to be in a table. That file is going to need a bunch of refactoring at some point. I tried to avoid the need for field_ui.js altogether (it's only purpose in this case is to dynamically change the options in the widget_type select list based on the type selected) by using #ajax but I couldn't quite get that to work.

So the first thing the form asks the user is to provide the type of field they want to create. Then, if there are already existing fields of that type, it asks the user if they want to reuse one of them or create a new one. If no existing field of that type exists, then the 'Add new' form elements are shown. This made sense to me but I might be the only one...

Another big thing that definitely isn't going to fly is that FieldAddForm extends OverviewBase. I did this initially to make it easier to do the conversion from the table-based layout. I feel like this should probably extend the FieldUI class and implement FormInterface- does that sound right? (UPDATE: Wrong.) Also in this patch, I moved getExistingFieldOptions() out of FieldOverview and into OverviewBase, but it should probably be moved into FieldUI. (UPDATE: also wrong.)

Note that this patch does not remove the existing Add-new/Reuse-existing table rows just yet and there are still some remnants of the table layout lying around- you'll probably notice the undefined index: parent_wrapper right off the bat.

xjm’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, field_ui-add-field-separate-task-1963340-30.patch, failed testing.

dags’s picture

Status: Active » Needs work

I'm following #1969728: Implement Field API "field types" as TypedData Plugins and am working a new patch based on those changes.

dags’s picture

In the meantime, we could use some feedback from the UI/UX/accessibility folks on the layout and interactions of this new FieldAddForm. Some questions to answer:

  1. How should the user interact with this form? Should they choose a field type first and then, only if a field of that type exists, are given the option to choose whether to use one of them or create a new one? Or should the first step just be to choose a New or Existing field?
  2. When choosing whether to use a new or existing field, should this be a button, select-list, radio, other? (I tried using an #ajax callback on a button but it seems that all buttons submit the form- no matter what properties are specified through FAPI. I think that's a known issue.)
  3. Should all select-lists use a dropdown widget or would radio buttons or some other widget/element be better?

I'm aware of the proposed style guide for seven, but is there any documentation on how to achieve newer D8-like styles through the Form API. It just seems to me like the styling of the widgets and form elements in my initial patch was pretty ugly. I was thinking that maybe there are some new #types or #attributes or class names that could be applied to them to make them look nicer.

xjm’s picture

@dags, could you add some screenshots of the interactions for review, and then tag the issue "Needs usability review"? Then one of our UX subject matter experts will know to give feedback.

mgifford’s picture

Status: Needs work » Needs review
FileSize
8.89 KB

re-roll mostly for element-invisible swap.

Status: Needs review » Needs work

The last submitted patch, field_ui-add-field-separate-task-1963340-36.patch, failed testing.

dags’s picture

@mgifford, the patch in #37 doesn't include the addition of the FieldAddForm. That's probably not intentional, right?

Are there any good resources/tools out there for creating D8 UI mockups?

xjm’s picture

@dags, when I have a working patch, I usually just take screenshots and annotate them with skitch.

mgifford’s picture

Status: Needs work » Needs review
FileSize
28.98 KB

Seems like the changes in core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php have already been added, so I removed that part of the patch. Should be reviewed!

Yes @dags, I totally missed the FieldAddForm file. I think I've got it re-attached.

@xjm is skitch only available via evernote? http://evernote.com/skitch/

Status: Needs review » Needs work

The last submitted patch, field_ui-add-field-separate-task-1963340-41.patch, failed testing.

dags’s picture

Status: Needs work » Needs review

@mgifford, thanks for the patch! could you also attach an interdiff between my patch in #30 and yours in #41? I'm having trouble seeing what's different at a glance.

I should have a few hours to work on this today. My goals will be to post screenshots and a new patch that cleans up all the sloppiness in the first patch.

dags’s picture

Status: Needs work » Needs review
FileSize
23 KB
26.19 KB

FieldAddForm now implements FormInterface and ControllerInterface instead of extending OverviewBase. I copied a couple of helper functions - fieldNameExists() and getExistingFieldOptions() - into the FieldUI class - modifying the later slightly so that it accepts $entity_type and $bundle as parameters. This is just a temporary solution. The previous patches probably would have passed but I had accidentally removed the parent_wrapper index from FieldOverview and not FieldAddForm as I intended - that's been fixed here. Screenshots coming later this afternoon - I have to work on something else for a little while.

PS. interdiff is against #41.

UPDATE: oops. forgot that interdiffs need to a .txt extension.

Status: Needs review » Needs work

The last submitted patch, interdiff.patch, failed testing.

dags’s picture

The patch in #44 creates a new FieldAddForm that renders like this:

Step 1:
field_ui-add-field-step-1.png

Step 2:
field_ui-add-field-step-2.1.png

Step 2 (alt):
field_ui-add-field-step-2.2.png

Step 3:
field_ui-add-field-step-3.1.png

Step 3 (alt):
field_ui-add-field-step-3.2.png

After clicking 'Save field settings' the user is taken to the field instance and field widget settings forms. Upon completion of these forms, they're redirected back to the FieldAddForm (this should be changed to redirect them back to the 'Manage Fields' tab).

In both cases (creating a new field or using an existing one), a user is asked to provide a label, widget, and weight. Weight isn't a whole lot of help and that field could easily be hidden and a default value used. When choosing to re-use an existing field, the only additional information the user is asked to provide is the name of the field they want to reuse. So this form could be simplified further - perhaps combining the 'New or Existing' and 'Existing field to share' dropdowns so that creating a new field is the default (empty option) until another field is selected?

As it stands, this form isn't particularly aesthetic and its a bit awkward. What other widgets could be used or styles applied to make this nicer?

dags’s picture

Status: Needs work » Needs review
FileSize
23.37 KB
48.55 KB
  • Removing add new/existing field rows from FieldOverview form
  • Removing related validation methods and submit code from FieldOverview class
  • Removing getExistingFieldOptions() and fieldNameExists() methods as they're no longer used by the class
  • Minor changes to PHPDoc comments on getExistingFieldOptions() in FieldUI class

Now the FieldOverview form looks like xjm's mockup in comment #1. Submitting it updates field weights by calling $entity_from_display->save().
manage_fields_redesign.png

Status: Needs review » Needs work

The last submitted patch, field_ui-add-field-separate-task-1963340-47.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
59.3 KB

This is really hard to review since so much of the code is just moved.
I'm attaching a diff rolled with more fuzzy context just for reviewing purposes by doing:
git diff --staged -C45

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -93,9 +87,6 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
-    // Field prefix.
-    $field_prefix = config('field_ui.settings')->get('field_prefix');

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUI.phpundefined
@@ -33,4 +33,70 @@ public static function getNextDestination() {
+    // Prefix with 'field_'.
+    $field_name = 'field_' . $value;

The "field_" prefix is now configurable. It doesn't look like this was fixed as part of that issue, but keep that in mind.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -124,6 +115,18 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+    $form['inline_actions'] = array(
+      '#prefix' => '<ul class="action-links">',
+      '#suffix' => '</ul>',
+    );
+    $form['inline_actions']['add'] = array(
+      '#theme' => 'menu_local_action',
+      '#link' => array(
+        'href' => 'admin/structure/types/manage/' . $this->bundle . '/add-field',
+        'title' => t('Add field'),
+      ),

Why is this needed here? Can't we do this with hook_local_actions() instead?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -733,64 +314,4 @@ public function getRowRegion($row) {
-  protected function getExistingFieldOptions() {

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUI.phpundefined
@@ -33,4 +33,70 @@ public static function getNextDestination() {
+  public static function getExistingFieldOptions($entity_type, $bundle) {
+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -733,64 +314,4 @@ public function getRowRegion($row) {
-  public function fieldNameExists($value) {

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUI.phpundefined
@@ -33,4 +33,70 @@ public static function getNextDestination() {
+  public static function fieldNameExists($value) {

Are these really better as static?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUI.phpundefined
@@ -33,4 +33,70 @@ public static function getNextDestination() {
+    $field_types = field_info_field_types();
...
+    foreach (field_info_instances() as $existing_entity_type => $bundles) {
...
+            $field = field_info_field($instance['field_name']);
...
+              && !field_info_instance($entity_type, $field['field_name'], $bundle)

These are all wrappers around OO code, let's use the OO bits and possibly inject what we can.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldAddForm.phpundefined
@@ -0,0 +1,560 @@
+    $instances = field_info_instances($entity_type, $bundle);
+    $field_types = field_info_field_types();
+    $extra_fields = field_info_extra_fields($entity_type, $bundle, 'form');

Same as before, this is injectable. Use the OO code it wraps

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldAddForm.phpundefined
@@ -0,0 +1,560 @@
+            'exists' => array(new FieldUI, 'fieldNameExists'),

This is not good. It was non-static before and could use $this. But if it stays static, it should be 'Drupal\field_ui\FieldUI::fieldNameExists'

dags’s picture

Status: Needs review » Needs work

Thanks for the review Tim!

I wasn't aware of git diff --staged -C45 - is this the preferred way to roll patches? Do patches made this way apply the same?

I'll work on using hook_local_actions() and replacing procedural wrappers with OO code.

The reason I made FieldNameExists() static is because it was previously declared and only used in the FieldOverview class. I suppose it could be moved into FieldAddForm and $this used instead, but I was thinking that other classes might want to use this method too. Either way though, you're right, "new FieldUI, 'fieldNameExists'" must go - so for now I'll move this method into FieldAddForm and remove the static call.

tim.plunkett’s picture

The -C45 trick just means git should consider files as a copy if 45% of them match.
It technically will still apply, but that's not preferred for our normal patch flow.
Just in this case it helped make the diff of the new class easier to read, to see what was changed while being moved.

YesCT’s picture

@dags Any update?

dags’s picture

I had almost finished the reroll a couple weeks ago before getting sidetracked on another issue. I'll try to finish it and post it this evening.

UPDATE: So that was a little ambitious. I started a new reroll against HEAD last night but then came across #2014821: Introduce form modes UI and their configuration entity and spent most of the time coming to terms with it. I think I've got my head around it now so I'll resume my work tonight.

Adding related issues for my own reference:
#1770720: [META] Gradual changes to Field UI
#2014821: Introduce form modes UI and their configuration entity
#1855992: Use dialog api for formatter settings
#552604: Adding new fields leads to a confusing "Field settings" form

YesCT’s picture

This might be blocking #1952394: Add configuration translation user interface module in core

Or... we could get #2045043: Field listings operations cannot be altered in and then incorpoate those changes into this one, since this one might be a lot harder.

yched’s picture

Getting #2045043: Field listings operations cannot be altered in separately, and then adjust the code if/when the "manage fields" is turned to a ListController, sounds fine by me ?

dags’s picture

Whew. This is my first substantial patch and I'm finding hard to keep up with HEAD.

I rolled the attached patch last Wednesday and had it working against 988abc2. It addresses some of the issues pointed out in #49 such as replacing wrappers with OO code. We're blocked by #2048969 for the hook_local_actions() issue. Also, I had to add back some JS code that was removed in #2014821 but I don't think the final patch should rely on this file -- I'm going to try to use the #states API instead.

The interdiff wouldn't be too helpful here so I'm omitting it.

amateescu’s picture

Thanks for staying on top of all these changes! :)

Also, I had to add back some JS code that was removed in #2014821

I assume that's needed for the widget selector? If so, you can safely remove it as we don't need to select a widget in the "field add" page anymore, it will just use the default widget.

falcon03’s picture

Status: Needs work » Needs review

Glad to see some progress here. Let's not forget that this is needed to improve the current Field UI accessibility; for details, see #1829368: A blind user cannot understand which form fields are required to add a new field or share an existing one.

Status: Needs review » Needs work

The last submitted patch, field_ui-add-field-separate-task-1963340-56.patch, failed testing.

xjm’s picture

Thanks @falcon03! Let's make sure to do a full accessibility review on the patch once it is ready.

falcon03’s picture

@xjm: sure. I'll do a full accessibility review as soon as the patch is ready. Honestly, I'm looking forward to doing it! ;)

agentrickard’s picture

Taking a look at #MWDS.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
31.4 KB

Reroll, courtesy of @YesCT.

amateescu’s picture

Status: Needs review » Needs work

The JS added in this patch needs to be removed, along with the widget selector. See #57 :)

agentrickard’s picture

Will do.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
25.69 KB

Let's try this one.

agentrickard’s picture

I do find it confusing that I cannot select the widget type from the Add/Edit workflow. Are we planning to add that back in?

amateescu’s picture

For editing a field, you have to the change the widget for a particular form mode, so a single screen with a widget selector doesn't cut anymore.

For adding a field, I think it's easier to bring it inline with the view modes workflow, for which you can't select the 'default' formatter.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-ux-reroll-66.patch, failed testing.

agentrickard’s picture

Fair enough.

All these test fails are expected, and I am working my way through them now.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
14.49 KB
38.49 KB

Fixed the tests, I think. I removed one test that was failing but was a duplicate of another test case.

Next to look at failing tests outside the 'field_ui' group.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-ux-reroll-71.patch, failed testing.

YesCT’s picture

Assigned: dags » YesCT

I'm working on fixing tests. patch to come shortly.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
5.37 KB
48.82 KB

serenaded tests [edit:] ran.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-ux-reroll-74.patch, failed testing.

YesCT’s picture

Issue summary: View changes

Updated issue summary.

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.87 KB
49.02 KB
221.62 KB

I missed one of the fields[_add... replacements

also

when adding a field manually to the contact form, after the first save field settings,
the next form gives (for the contact rrrrrr):
Notice: Undefined index: rrrrrr in Drupal\field_ui\Form\FieldInstanceEditForm->buildForm() (line 103 of core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php).j

    drupal_set_title(t('%instance settings for %bundle', array(
      '%instance' => $this->instance->label(),
      '%bundle' => $bundles[$entity_type][$bundle]['label'], //line 103
    )), PASS_THROUGH);

The entity type is coming up node, and there is no contact rrrr node type. So the "Add field" link url is wrong.

contact-entity-type.png

@fubhy pointed me to
in FieldOverview.php
tried changing to this first:
'href' => 'admin/structure/' . $this->entity_type . '/manage/' . $this->bundle . '/add-field',
is not right, that gives me /contact_message/

admin_path? yeah.

still work to do.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-76.patch, failed testing.

agentrickard’s picture

FileSize
44.06 KB

Straight reroll from #76 before trying to fix tests.

agentrickard’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
45.24 KB

And this test should work. I simplified the click-behavior testing of the contact form, which really isn't necessary.

tim.plunkett’s picture

I remember that test coverage being explicitly added. clickLink() helps us ensure the UI actually works, not just the forms with drupalPost().

Berdir’s picture

Yes, everyone wants to remove my test ;) We should add a comment and explain why it's there.

agentrickard’s picture

That test is very hard to follow, does not follow any other test patterns in core, and tests a UI that should be covered by Field UI tests.

Berdir’s picture

It can't be covered by Field UI test, it's a regression test for a contact category specific bug because contact categories were not correctly integrating with field UI and those links were no longer displayed. There is only very little glue code remaining there as of today (bundle_of = "contact_message") but I think it's still useful to have.

agentrickard’s picture

I'm not willing to argue about those tests in this patch. Restored.

swentel’s picture

FileSize
6.56 KB
44.6 KB

Reroll after entity storage and some other patches got in. Not entirely sure whether it will pass, but we'll see.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-85.patch, failed testing.

Hydra’s picture

Assigned: Unassigned » Hydra
Status: Needs work » Needs review
FileSize
45.63 KB

I tried creating an interdiff, but I "accidentally" fixed a lot of things when I was manually rerolling the patch.. :/

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-87.patch, failed testing.

Hydra’s picture

Status: Needs work » Needs review
FileSize
45.67 KB
1.38 KB

Okay this should dix the test now.

mgifford’s picture

FileSize
61.32 KB
86.62 KB
62.18 KB
71.21 KB

Seems to work. However after you've set up the field you just go back to:
admin/structure/types/manage/article/add-field

Very useful if you want to add more than one field. But the D7 behavior (and what I think should be the default behavior), is to send folks back to:
admin/structure/types/manage/article/fields

It could be that we want to add a button at the end that says something like "Save" "Save and add another field"

However, Save should go back to the list of fields for that content type.

Add Field Button

Add Field Screen

Edit Field

Save Field

It's this last screen that's the problem.

mgifford’s picture

Status: Needs review » Needs work

Forgot to change the status.

pguillard’s picture

Here is my suggestion :
Since FieldInstanceEditForm/submitForm() is called at the last step, I think we can force redirection to /fiels instead of retreiving getNextDestination().

Info : I'm still newbie in core...

pguillard’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Needs work
+++ b/core/modules/number/lib/Drupal/number/Tests/NumberFieldTest.php
@@ -163,11 +163,11 @@ function testNumberIntegerField() {
diff --git a/index.php b/index.php

diff --git a/index.php b/index.php
index 426aa22..bc1f2ef 100644

index 426aa22..bc1f2ef 100644
--- a/index.php

--- a/index.php
+++ b/index.php

+++ b/index.php
+++ b/index.php
@@ -7,7 +7,9 @@

@@ -7,7 +7,9 @@
  * All Drupal code is released under the GNU General Public License.
  * See COPYRIGHT.txt and LICENSE.txt files in the "core" directory.
  */
-
+error_reporting(E_ALL);
+ini_set('display_errors', TRUE);
+ini_set('display_startup_errors', TRUE);
 require_once __DIR__ . '/core/vendor/autoload.php';
 require_once __DIR__ . '/core/includes/bootstrap.inc';

This should not be part of the patch ;)

pguillard’s picture

Status: Needs work » Needs review
FileSize
801 bytes
46.45 KB

This time should be correct with a correct interdiff. Sorry.

Status: Needs review » Needs work

The last submitted patch, field_ui-1963340-95.patch, failed testing.

rteijeiro’s picture

FileSize
94.12 KB

After applying the patch, I created a new field and get the following error:

Notice: Undefined variable: destination in Drupal\field_ui\Form\FieldInstanceEditForm->submitForm() (line 193 of core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php).

add-field.png

pguillard’s picture

Status: Needs work » Needs review
FileSize
46.4 KB
875 bytes

I did a bad mistake rerolling my patch, copy pasting wrong line. This one should be ok now!

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Patch #1963340-98: Change field UI so that adding a field is a separate task seems to work like a charm. Compared with #1963340-89: Change field UI so that adding a field is a separate task patch and includes all the code.

I tested it creating a new field and after that it redirects me to the fields list page as expected.

Hope somebody can take a look deeper in the proposed solution but it looks right for me.

dags’s picture

I think this accomplishes the goal of "changing field-ui so that adding a field is a separate task." The UX of adding a new field still needs work IMO, but that's more of an issue related to #1770720: [META] Gradual changes to Field UI.

Grayside’s picture

Should a select element be used for two options?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.86 KB
46.49 KB

Re-roll
Also label should be populated with $this->randomString()

Status: Needs review » Needs work

The last submitted patch, drupal8.field-ui.module.1963340-103.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
47.8 KB
1.31 KB

Fix the rest

agentrickard’s picture

Status: Needs review » Reviewed & tested by the community

Small changes in the re-roll. Should still be RTBC.

klonos’s picture

...just a reminder that once we get this issue fixed we should file a follow-up to add a "Save and add another field" button as suggested in #90 by mgifford.

andypost’s picture

Also it could be good UX to use list of field types (as node/add does) when there's less then 10-15 field types

andypost’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal8.field-ui.module.1963340-109.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
59.46 KB
19.63 KB

fixed patch:
- use FormBase
- title set on route

andypost’s picture

Minor clean-up. Still needs work.

field_ui-add.png

andypost’s picture

Issue tags: -Field API +Entity Field API

Also 'Save' button should be removed in favor of ListController suggested by @yched in #55

PS: pushed to 1963340-field-ui.add branch of https://drupal.org/sandbox/yched/1736366

LewisNyman’s picture

This is looking like a nice improvement, it cleans up the manage fields page but it needs work to make sure we don't regress the usability of adding a field.

General comments

The “add existing field” flow feels very hidden and confusing. Do I need to remember the type of field it is before I see it? Edit: Now, I can't find this option at all, has this been removed in D8?

“Add existing”, “Create”, and “Configure” feel very similar to actions in the Blocks UI. It would be nice if can share mental models between the two.

Specific Comments

The machine name autocomplete widget looks different to the content type machine name widget. It would be nice to get these consistent. I've opened an issue to add this component to the Seven Style Guide #2108079: Document machinename.css with CSS comments

Screen Shot 2013-10-09 at 11.34.33.jpg

Screen Shot 2013-10-09 at 11.37.24.jpg

The field settings screen needs a primary button. I'm not a huge fan of the “Unlimited/Limited” selection widget but I think that might be another issue. Maybe it should default to “Unlimited”?

Screen Shot 2013-10-09 at 11.46.29.jpg

Xano’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldAddForm.php
@@ -177,24 +158,64 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+    $form['type'] = array(
+      '#type' => 'select',
+      '#title' => $this->t('Type of new field'),
+      '#title_display' => 'invisible',
+      '#options' => $field_type_options,
+      '#empty_option' => $this->t('- Select a field type -'),
+      '#description' => $this->t('Type of data to store.'),
+      '#attributes' => array('class' => array('field-type-select')),
+    );

Now this element no longer lives in a table, we should get rid of the description and display the title, which may be rewritten to "Field type". The same goes for the field label.

Xano’s picture

Issue summary: View changes

Removed totally unrelated issue.

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 112: drupal8.field-ui.module.1963340-112.patch, failed testing.

Bojhan’s picture

Assigned: Hydra » yched
Issue summary: View changes

Lets have yched take a look at this.

amateescu’s picture

Assigned: yched » amateescu

Working on this.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_DECEMBER
FileSize
89.46 KB
74.29 KB

Here's what I have so far, thanks to the sprint in London this weekend.

Here's a summary of this patch/interdiff:

TL;DR: A crap ton of code cleanup :)

  • The FieldAddForm class has been mostly rewritten so there shouldn't be any leftovers of the table rows-based code we had in FieldOverview
  • I kept the states approach for showing a new form element if the user selects a field type which can be "shared" but I changed it to radios so the choice is more obvious and descriptive
  • Changed the "step two" state to an ajax operation, so we ca show only the relevant fields that can be shared
  • Created a FieldInstanceListController and removed FieldOverview
  • Merged OverviewBase into DisplayOverviewBase since that was the only class that extends it now
  • Addresssed @LewisNyman's review from #114

All the tests will need to be fixed again but I would *strongly* prefer to have another round of UX and code reviews for the new FieldAddForm before starting to fix them..

andypost’s picture

#120 is a tremendous clean-up and code decoupling that will allow to simplify #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects

Ajax loading really helps to hide unused fields but...

- user still needs to check each field type to find the field existing field, not sure that that site builders should operate the type=>name way, suppose better to have list of existing fields separate
- form breaks sometimes ( ajax load existing fields and then try to change type to other)
- when type is changed after ajax load the machine name stays visible and filled with previous value

amateescu’s picture

user still needs to check each field type to find the field existing field, not sure that that site builders should operate the type=>name way

My reasoning for keeping the process this way is something like "Users don't need to know that they can re-use an existing field before selecting a field type."

when type is changed after ajax load the machine name stays visible and filled with previous value

Yeah, I noticed this in my testing, but it's a bug with the machine_name form element which doesn't act appropiately if a form was updated via AJAX, so separate issue :)

yched’s picture

Merged OverviewBase into DisplayOverviewBase

w00t! ++
Looked in there recently for #2144919: Allow widgets and formatters for base fields to be configured in Field UI, and thought that OverviewBase had definitely no reason to exist anymore :-)

The last submitted patch, 120: 1963340-rewritten.patch, failed testing.

amateescu’s picture

FileSize
88.62 KB
11.51 KB

form breaks sometimes ( ajax load existing fields and then try to change type to other)

Apparently, the #states approach doesn't work very well so I switched the field type select to #ajax instead.

The last submitted patch, 125: 1963340-rewritten-125.patch, failed testing.

amateescu’s picture

FileSize
0 bytes
786 bytes

Updated the patch to account for #2152581: "Manage fields" screen needs mobile-izing.

Also discussed a bit with @yched and we agreed that this needs a UX review first because the code can change a lot based on that.

Bojhan, will you do the honors? :)

Bojhan’s picture

Is this truly ready? I have RTBC patches too much lately, that ended up not working well.

amateescu’s picture

FileSize
88.55 KB

@Bojhan, it's ready for UX review of the approach. Code review, test fixing and RTBC comes later...

Attaching a non-empty patch this time, which should not be tested by the testbot as we already know it needs work.

amateescu’s picture

FileSize
90.38 KB

Rerolled. Is there any chance to get a UX review here or we should carry on with code review and test fixing?

Bojhan’s picture

This patch doesn't apply and creates a error on initiation?

amateescu’s picture

FileSize
90.56 KB
7.64 KB

You're right, the patch needed some more updates to account for #2005716: Promote EntityType to a domain object. This one applies and works well on latest 8.x.

mgifford’s picture

I applied it locally and it worked. Installed it on simplytest.me and I got this error:

Fatal error: Cannot use object of type Drupal\Core\Entity\EntityType as array in /home/s0821502d3118ffa/www/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiLocalAction.php on line 65

amateescu’s picture

Yep, #132 fixes that error.

Bojhan’s picture

It seems like it is still broken.

1) There is no label above type,

2) Machine name does not get autocompleted.

3) Also, I would really like a "Save and continue" button rather than just save, when you are in a wizard kind of flow. Now that there are 3 steps, its more and more difficult.

amateescu’s picture

FileSize
90.52 KB
1.1 KB

Thanks for starting to review :)

1) Added a 'Field type' label.
2) This is an existing bug with the machine_name element, which loses its state/behavior after an AJAX operation on a form.
3) Fixed.

amateescu’s picture

Issue tags: +DrupalWorkParty
FileSize
91.71 KB
Xano’s picture

FileSize
84.23 KB

Re-roll.

@amateescu: I assume the sprint tags can be removed?

amateescu’s picture

@Xano, I don't know, do they usually get removed after a while?

And the patch size has decreased dramatically, 84K vs. 91K, are you sure you didn't miss anything? Note that this work is tracked in the 1963340-field-ui.add branch of @yched's sandbox and it would be helpful to keep it there for easier merges in the future.

Edit: I also cancelled the test because the latest approach is not ready for the testbot.

Status: Needs review » Needs work

The last submitted patch, 138: drupal_1963340_138.patch, failed testing.

andypost’s picture

Just wanna to point again after IRC discussion with amateescu

Let's re-iterate on this, because adding new fields is one of really often tasks and we should care about time that we will lost by clicking within UI and waiting some AJAX to complete while d6 and d7 we have "full entity picture"

<andypost> amateescu, is it possible to skip ajax for this?
<amateescu> andypost: nope
<amateescu> andypost: we have update select elements so #states don't cut it
<andypost> amateescu, I still think that better to preload all data 
<amateescu> nod started looking into the machine name bug then he forgot about it :(
<andypost> amateescu, suppose it would be much more usable to have 2 action links to add existing and new field
<amateescu> andypost: hm.. didn't think about that
<andypost> amateescu, I spent some time to gathering opinions about that and most of people confused  this form
<amateescu> andypost: were they confused because the option to reuse an existing field only appeared when selecting some specific fields?
<andypost> amateescu, yes
<andypost> amateescu, at this new screen you NOT see what fields already added so people needs to switch tab to return to context
<amateescu> andypost: right
<amateescu> andypost: can you post that in the issue? maybe Bojhan agrees with you :)
amateescu’s picture

Status: Needs work » Needs review
FileSize
91.92 KB
3.38 KB

Thanks @andypost, this means we need more usability testing here. In the meantime, here's a proper reroll.

amateescu’s picture

FileSize
91.94 KB
10.65 KB

Missed some core changes.

swentel’s picture

Berdir also suggested of storing things in form_state until all settings are configured - if possible at all. He was testing with a new field type which extended from entity reference and that completely blew up right now because the target_id wasn't properly configured.

yched’s picture

Berdir also suggested of storing things in form_state until all settings are configured - if possible at all

Not sure what that refers to exactly, but one possible blocker is that it's currently impossible to build an Instance object for a Field that doesn't exist yet in real config - FieldInstance::_construct() fetches a Field from field_info_*().

So it's currently impossible to display the "Instance settings form" step before the Field has actually been created.

Would be nicer if FieldInstance allowed injecting an arbitrary runtime Field object in its construct. I started looking into this shortly after the "field / CMI" patch landed last spring, but it turned into a bit of a rabbit hole back then. Maybe things have changed a bit since then.

Berdir’s picture

My problem was the target_type configuration, right now it's required to have a default simply to be able to initially create the schema for that and then throw it away and re-create it after you've configured the field type settings.

I guess the expected way would be to save the field after the field settings step and then create the instance for the instance settings page? Not sure how easy it is, but the current behavior is quite fragile.

yched’s picture

#146: ok, right - so, not strictly related to Instances & #145, and true, the current is less than ideal :-/.

star-szr’s picture

Bump, is this still a viable path? Was just re-checking #1938900: Convert theme_field_ui_table into a template.

amateescu’s picture

I guess it is, but the patch has to be rewritten mostly from scratch, too many things changed in these 8 months.

xjm’s picture

Issue tags: +beta target

Yeah, it would be great if we could still do this. It will definitely take some work to get an updated patch.

Berdir’s picture

Status: Needs review » Needs work

This would probably be a pretty huge UI/form structure change, can we still do that? Or asked differently, is the issue important enough to do it?

webchick’s picture

We haven't locked down the UI of D8 yet, and this is fixing a major UX bug so yes, I think it's still on the table if it gets done by RC.

andypost’s picture

Talked with @Bojhan in AMS about that he said we need to fix block placement first and then re-use the same pattern for field list form

PS: the related issue is about how to provide a hook_help() for core's fields and a way to re-do this in a useful way

Bojhan’s picture

@andypost Do keep in mind thats long-term, unless you see an immediate way to fix that. This issue is a intermediate step which is great and can be done now.

amateescu’s picture

Status: Needs work » Needs review
FileSize
66.46 KB

RC is just around the corner so let's restart this! The patch has been rewritten from scratch and implements a new approach: both 'select a field type' and 're-use existing field' selectors are visible from the start, no more funky AJAX monkey business.

Also opened #2367665: Add primary actions on the 'Field storage settings' and 'Field settings' forms for @LewisNyman's review from #114.

amateescu’s picture

FileSize
66.41 KB

Rerolled.

mgifford’s picture

Thanks @amateescu - looks like you've moved it ahead a ways.

For the next 5 hours there's an instance up on simplytest.me

screen capture of adding a new field.

LewisNyman’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
66.41 KB

Done :)

Status: Needs review » Needs work

The last submitted patch, 159: 1963340-159.patch, failed testing.

LewisNyman’s picture

Issue summary: View changes
FileSize
468.62 KB

This is looking great, I think it would work better if the label fields was first, above the field type fields.

Andrei also told me he was planning on loading the field settings form via AJAC once the field type is selected, that sounds really good because I felt like I lost the context of the field I was editing by the second page.

amateescu’s picture

Status: Needs work » Needs review
FileSize
66.37 KB
2.75 KB

Thanks Lewis! Easy fix for #161.

LewisNyman’s picture

+++ b/core/modules/field_ui/css/field_ui.admin.css
@@ -3,6 +3,16 @@
+/* Add new field page. */
+.field-ui-add-field .field-type-wrapper .form-item {
+  float: left;
+  margin-right: 1em;
+  vertical-align: text-bottom;
+}

I wonder if we can use container-inline for this instead of new CSS?

amateescu’s picture

I tried that initially but container-inline also puts the labels inline and it doesn't look that good anymore :/

Bojhan’s picture

Lets go ahead and fix this issue mentioned by lewis, it seems like we are on track to get this in!

amateescu’s picture

Ok then, since we have approval from Lewis and Bojhan for the current approach, I'll go ahead and start fixing tests (yay...).

xjm’s picture

Issue summary: View changes

Evaluating this in terms of the allowed beta changes policy (https://www.drupal.org/core/beta-changes):

Issue category Task (not a feature) because the form already exists, and the patch does not add or remove any functionality.
Issue priority Major because the Field UI is one of the most important site builder UIs, and it consistently trips users up in usability studies. This change also will make further UX improvements to the Field UI more feasible. It is not critical because the same UI is in Drupal 7.
Prioritized changes The main goal is to improve both the usability and accessibility of this important form, so it is a prioritized change for the beta phase.
Disruption
  • This change will require rework to existing modules that alter the manage fields form.
  • Many tests will need to be updated because they include assumptions about the current UI.
  • The disruption to core will be limited to the Field UI itself.

I think the impact of this change is clearly greater than the disruption, so this is still a great issue to complete during the beta. @webchick also approved the change in #52. Thanks @amateescu for working on this!

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
amateescu’s picture

FileSize
94.18 KB
35.41 KB

@xjm, cool! Here's a green patch to go along with that evaluation.

yched’s picture

FileSize
26.39 KB
25.45 KB

Thanks a lot for keeping at this, @amateescu ! We should definitely try to drive this home.

After a first round of testing, I think there's an issue with the "label" input.

When you select "re-use an existing field", you need to enter a label :

Fine. We used to have some JS code to auto-fill the "Label" input with the label of the selected existing field, but apparently this was removed at some point - and #1426804: Allow field storages to be persisted when they have no fields. is removing the "label of the existing field" from the dropdown anyway.

But filling that label auto-generates a "machine name" :

That is wrong, since in the "re-use existing" case, you won't be creating a new field name, but reuse the existing one, and so the generated machine name will just not be used.

I'm afraid the two cases ("add new" / "re-use existing") cannot share the same "Label" input ?

Those two cases are different enough that IMO :
- either they would deserve two different "add" buttons on the initial "field overview" page, pointing to two separate forms
- or, if one single button pointing to one single form, that form should start with a radio choice between "add new" or "re-use existing", with #states or ajax refreshing of the rest of the form.

I guess the latter could be nicer UX if the difference between "add new" & "re-use existing" is not clear for the user (avoiding the "blue pill / red pill" effect by making it clearer what the two choices actually mean) ?

yched’s picture

The two proposals in #171 would also avoid having to deal with the case of "user choses an option in both selects", which currently gives the obviously-needs-work "So.. you want to add a new field *and* re-use an existing one? Not cool!" error message :-)

Bojhan’s picture

What about two entry points, one for a new field one for reusing an existing one? @yched what are your thoughts on that, is this possible? It would eliminate this problem.

amateescu’s picture

- either they would deserve two different "add" buttons on the initial "field overview" page, pointing to two separate forms

This would definitely simplify the code a lot, but, as you note later, it requires users to think too much on the consequence of clicking one of the action links, and even browing back and forth between them until making a final decision.

- or, if one single button pointing to one single form, that form should start with a radio choice between "add new" or "re-use existing", with #states or ajax refreshing of the rest of the form.

This was attempted in earlier versions of the patch and it had the same problems as the above, the user has to experiment a bit (while waiting for ajax refreshes in the meantime, which also breaks the machine name element) before coming to the conclusion that X and not Y is the better thing to do.

I'd propose to just hide the field name when something gets selected in the 'existing field' dropdown. I even tried this while working on rewriting the patch but the machine name element doesn't play nice with #states either.

As for the auto-filling the "Label" input, it's still there in the current UI but I looked at the js code and I just can't understand anything from it. It was definitely a lot more readable in D7 when we were using plain jQuery syntax.

which currently gives the obviously-needs-work "So.. you want to add a new field *and* re-use an existing one? Not cool!" error message :-)

Sigh.. I was hoping this easter egg would be discovered much later :)

amateescu’s picture

Oops, cross-posted. @Bojhan, Yves suggested that in his comment and I also replied to that suggestion.

If everyone thinks two entry points are fine then I don't have any strong opposition since it will make my life easier as well.

yched’s picture

Yes, agreed with the drawbacks of "2 separate entry points" :

it requires users to think too much on the consequence of clicking one of the action links, and even browing back and forth between them until making a final decision.

IMO the best UX would be :
- One form, 1st choice being a radio between "add new" and "reuse existing". This lets us put guidelines / help in each option's #description
- Each option reveals a subform using #states (no ajax)
"add new" : label textfield with machine name + dropdown select for field type. When a field type is selected, the "field type storage settings" form is ajax-embedded.
"add existing" : dropdown select for existing field - no need for a "label" input here, hitting Submit redirects you to the edit form for the field you just added, you can set the label there ?

amateescu’s picture

Re #176: And would the first choice be selected initially (i.e. the "add new field" subform is already displayed). Or.. is it ok to not have any radio option selected by default?

Also, are there any known accessibility problems with Form API's #states functionality? We have to keep in mind that we're not only trying to improve usability here but also accessibility.

yched’s picture

And would the first choice be selected initially (i.e. the "add new field" subform is already displayed)

Good question - I'd think yes, given that this is the most frequent use case ?

Also, are there any known accessibility problems with Form API's #states functionality?

Good question too - I don't know about that though. Maybe others know ? @Bojhan ?

Bojhan’s picture

There should be no accessibility issues with it, I am quite sure of that.

amateescu’s picture

Are there any other opinions on the approach suggested in #176 before I start implementing it?

amateescu’s picture

FileSize
94.91 KB
8.13 KB

Discussed this with @yched during the Entity Field API meeting a few days ago and we agreed to do this for now:

- since we cannot auto-populate the label when the user selects an existing field anymore (thanks to #1426804: Allow field storages to be persisted when they have no fields.), we need to hide the label and field_name form elements when the user selects something from the 're-use existing field' dropdown
- because of that, the label cannot be the first form element anymore since the row with the two selects will jump up and down depending on whatever is selected in either of them
- we can provide an additional visual indicator that the user can not select a new field type and re-use an existing one at the same time by automatically unsetting the other option when one of them is changed

I know that both @LewisNyman and @Bojhan preferred to have the label element on top but we need to take into account that the label only makes sense for the 'add new field' operation. And also that we will show even more form elements below the label when the user selects a field type in [552604], which will basically bring the "field storage form" in this screen via AJAX.

yoroy’s picture

Status: Needs review » Needs work

Approach in #176 sounds great :)

yoroy’s picture

Status: Needs work » Needs review

ha, posted at almost the same time :)

Status: Needs review » Needs work

The last submitted patch, 181: 1963340-181.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

I refuse to update all those tests for each new iteration of this patch so I opened #2384357: Simplify Field UI testing and I'll appreciate a quick RTBC :)

Meanwhile, the patch in #181 works just fine and the new approach can be reviewed manually.

amateescu’s picture

FileSize
87 KB
5.61 KB

That issue got in so we're back to sanity here. Now I'm willing to trade gold for UX reviews and a code review from @yched :)

yoroy’s picture

FileSize
41.49 KB
29.6 KB
46.45 KB
70.74 KB
31.59 KB

This is great!

I tested this on simplytest.me for a couple of different field types. It all seems to work quite well :)

Initial screen presents only a select list of available field types:

After selecting a field type, the text field for adding a label is shown immediately. Made perfect sense to me:

Errors work too! (I hit the back button from the next screen back to this one, then submitted again):

The next step is the specific field settings screen, which is what we already know and still works as ever:

I do have one question:

This second select list for reusing existing fields only started to show up after adding a couple of fields. I had been adding new fields to the Article content type and only after switching to the Page content type and adding new fields there did I start to see the 'reuse' select list. From that moment on, also on the Article content type.

I had a look at how that works on D7 and it shows the same behavior. It's a bit strange because it's hard to tell which change makes the 'reuse' select list show up. It's just made more apparent through this new UI. So, probably not in scope of this issue to do something about that.

I asked an experienced developer collegue here to add a date field to the Page content type and he succeeded without problems. He couldn't really tell what had changed (this is a good thing). His response: "Not worse", which is Belgian for "pretty good" :-)

yoroy’s picture

FileSize
22.08 KB

When reusing an existing field you do not get the label text field. But on the next screen you *can* alter the label. The machine name is filled in as the default name. Is there a reason you can't give a label immediately when reusing an existing field? It's a bit weird in the interaction now. From adding a couple of new fields first, you are trained to expect that field label text field to pop up. When you select an existing field there's no clue that you can go ahead and push the submit button. Ideally, let me put in the label just like with new fields.

Wim Leers’s picture

It also seems strange to me that you first enter a non-required label here:

And then see the label you entered in the next step again, but this time it's required:

Berdir’s picture

I had a look at how that works on D7 and it shows the same behavior. It's a bit strange because it's hard to tell which change makes the 'reuse' select list show up. It's just made more apparent through this new UI. So, probably not in scope of this issue to do something about that.

Yes, it can not work in a different way. You can't re-use the same field multiple times on the same node type, so you can only re-use them on different ones.

It was slightly different in 7.x because you saw all the fields of all the entity types there, so you were more likely to see some fields already from the start.

Re-using is the same field, so the machine name has to be the same.

I guess we could make the label field visible with the label pre-filled but it will probably require some JS trickery :)

Sounds like what you are thinking of is more a duplicate-as-a-new-field thing.

yoroy’s picture

Thanks, that explanation makes sense. So the first label is not really the same as the second label. First one is the name of the field, second label is the label-label for this instance of the field? Is that what's confusing me?

Berdir’s picture

It is pretty much the same thing.

But when re-using fields, in most cases, you want to use the same label (Because it is the same thing. For example, you have the same image field on 3 different node types, you're not going to name it Image on one of them and Banana on the other :)). So we optimize for that case and use that label by default.

Technically, on the first step, you create the storage of the field, if you use an existing field, you use the same storage definition and then copy the field definition (that is per entity-type) and optionally change it. And the label is on the field definition, the storage definition has no user-provided label.

But the label problem can be solved, I think we can always make the label field visible after you selected a field and then in case of an existing field, pre-fill it with an (existing_field_name => existing_field_label) map. Once we do that, we can also mark it as required, because it really is (we could also do that with #states) @amateescu?

Theoretically, the field label is, as mentioned above, only required on the second step, so we *could* only ask for it on step two. But I assume that's now how users think about it.

yoroy’s picture

Thanks again. So yes, we should show a pre-filled textfield when reusing an existing field. It's mostly about consistency in behavior compared to adding a new field, especially because not showing anything is not enough to indicate that you can go ahead and submit.

And yes, mark it required. Thanks!

amateescu’s picture

@Berdir, the only problem with the label is that since #1426804: Allow field storages to be persisted when they have no fields. got in, it is possible to have a field storage without any field instance, so we don't have any label to default to. But I guess we can work around this case by defaulting it to the field name, just like the current "second step" does for re-using existing fields.

So yes, the label problem is easily solvable, but if we make it required for both adding a new field and re-using an existing field, we would have to move it back on top of the two selects instead of showing it below after something is selected?

yoroy’s picture

Status: Needs review » Needs work

First I choose this puppy dog, only after I chose do I give it a name. I don't see why the order should be changed.

amateescu’s picture

@yoroy, because #161 :P I also prefer to have it below (that's why it was implemented like that initially), but I don't really care that much so you folks will have to decide :)

LewisNyman’s picture

hmm yes. It's hard to tell if it's just the bias of the current UI that makes it feel awkward.

Here's a justification, the user in this case is usually a developer, content creators don't tend to create site structure. From my point of view developers think like this:
$identifier = new Object

Bear in mind that the label also generated the machine name, it feels natural to decide the identifier first in this context. Maybe some develoeprs will disagree with me :-). I';; leave Yoroy to make the call.

yoroy’s picture

Ha, yeah, but even in the current Field UI I first select the field type from the select list, then move to the left to name it. This isn't a case of one way is right and the other wrong. I don't think we should model from a developer mindset (and @amateescu is a developer and likes it below :-)

Lets leave it below as it's implemented now.

amateescu’s picture

My reasoning for keeping it below is that when I want to add a field, I don't necessarily know beforehand what field type to use (or re-use) and the label I give it is usually influenced by that decision :)

Anyway, since I feel we're chasing our tails a bit for the past 40 comments or so, and I'd love to have a solid plan until I implement the changes tonight, I have one last question:

Given that we decided (I hope?) to leave the label below, make it required and that we also have to show and populate it on a best-effort basis when chosing to re-use an existing field, do we want to keep the current behavior of having it hidden until something is selected in one the of dropdowns?

yoroy’s picture

do we want to keep the current behavior of having it hidden until something is selected in one the of dropdowns?

Yes! :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
90.23 KB
20.62 KB

Here we go, this patch implements all the points discussed above. The JS part wast quite tricky but I think I managed to find and take care of all the corner cases.

amateescu’s picture

FileSize
90.59 KB
5.51 KB

Well.. this is embarrassing, not following my own protips that I post on twitter :) And a small cleanup that I thought I already did in the previous patch.

yoroy’s picture

Assigned: amateescu » yched

Doing the not-so-subtle thing and assigning to yched for review :)

swentel’s picture

Went through it manually and this works really nice.
Can't really find anything in the code that bothers me for now, let's wait for yched :)

yched’s picture

Yay, patch is full of awesome, thanks a ton @amateescu !

I do have remarks, but nothing big.

  1. I pointed a couple behavior errors when you change between "select a value in 'add new'" and "select a value in 'add existing'" direclty with @amateescu at the sprint venue.
  2. On the "Add field" page, the last breadcrumb link should be the "Manage fields" page ?
    The breadcrumb trail currently stops one step before that.
  3. +++ b/core/modules/comment/comment.module
    @@ -311,17 +311,18 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
    +    // @todo This looks very brittle, we should implement hook_field_presave()
    +    // instead.
    

    True, but the patch does not make it worse so, not related ?

  4. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
    @@ -55,11 +55,11 @@ public function testFieldAdminHandler() {
    @@ -192,11 +192,11 @@ public function createEntityReferenceField($target_type, $bundle = NULL) {
    

    Couldn't this helper function just use the FieldUITestTrait now ?

  5. +++ b/core/modules/field_ui/css/field_ui.admin.css
    @@ -3,6 +3,16 @@
    +.field-ui-field-storage-add-form .field-type-wrapper .form-item {
    +  float: left;
    +  margin-right: 1em;
    

    We probably need to add a RTL version somewhere ? Not sure how this is done these days :-)

  6. +++ b/core/modules/field_ui/css/field_ui.admin.css
    @@ -3,6 +3,16 @@
    +.field-ui-field-storage-add-form .field-type-wrapper .form-type-item {
    +  margin-top: 2.3em;
    +}
    

    Very unclear that this targets the "or" separating the two selects :-)
    Could we add a specific class to make the selector clearer ?

  7. +++ b/core/modules/field_ui/field_ui.js
    @@ -3,10 +3,59 @@
    +  Drupal.behaviors.fieldUIFieldStorageAddForm = {
    +    attach: function (context) {
    +      var $fieldType = $(context).find('select[name="type"]');
    +      var $existingField = $(context).find('select[name="existing_field_name"]');
    +      var $fieldLabel = $(context).find('input[name="label"]');
    +      var $machineName = $(context).find('.form-item-label .field-suffix');
    +      var labelValue;
    

    Looks like this is lacking some .once() logic, to make sure the event listeners don't get attached several times if behaviors get attached several times.

  8. +++ b/core/modules/field_ui/src/Controller/FieldConfigListController.php
    @@ -0,0 +1,39 @@
    +class FieldConfigListController extends EntityListController {
    

    YAY!

    Although - at some point we might want to show base fields and / or BaseFieldOverrides there, which wouldn't play nice with the simple "entity list", but well, we'll see when we get there :-)

  9. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -16,13 +16,50 @@
    +   * The entity type of the entity bundle.
    +   *
    +   * @var string
    +   */
    +  protected $bundleEntityType;
    
    +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,512 @@
    +  /**
    +   * The entity type of the entity bundle.
    +   *
    +   * @var string
    +   */
    +  protected $bundleEntityTypeId;
    

    Yeah those $this->bundleEntityTypeId are really ugly :-/

    This is only needed by code that needs to generate links/routes for field UI pages, I think we could make that smarter, but that's not for this issue.

    The phpdoc text is a bit gibberish though :-)

  10. +++ b/core/modules/field_ui/src/DisplayOverviewBase.php
    @@ -118,8 +184,19 @@ protected function getFieldDefinitions() {
    +    $stored_bundle = $form_state->get('bundle');
    +    if (!$stored_bundle) {
    +      if (!$bundle) {
    +        $bundle = $this->getRequest()->attributes->get('_raw_variables')->get($this->bundleEntityType);
    +      }
    +      $stored_bundle = $bundle;
    +      $form_state->set('bundle', $bundle);
    +    }
    

    really weird dance with $stored_bundle / $bundle / $form_state['bundle'] - any way we can simplify this ?

  11. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
    @@ -32,27 +65,104 @@ class FieldConfigListBuilder extends ConfigEntityListBuilder {
    +    $this->fieldTypeManager = $field_type_manager;
    

    That property is not documented as a class member :-)

  12. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
    @@ -32,27 +65,104 @@ class FieldConfigListBuilder extends ConfigEntityListBuilder {
    +    $this->targetEntityTypeId = $target_entity_type_id;
    +    $this->targetBundle = $target_bundle;
    +    $this->targetBundleEntityType = $this->entityManager->getDefinition($target_entity_type_id)->getBundleEntityType();
    +    $this->fieldTypes = $this->fieldTypeManager->getDefinitions();
    

    Yeah, it's awkward that render() is the place where we initialize object properties for the rest of the methods, because we can't do it in the __construct() :-/

    It works because we know render() is the entry point and the rest is not called before that, but that's really not a great pattern.

    Could we at least remove $this->targetBundleEntityType and $this->fieldTypes as properties on the object, and grab the data in the places we really need it ?
    Looks like $this->fieldTypes at least could be easily removed ?

  13. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
    @@ -32,27 +65,104 @@ class FieldConfigListBuilder extends ConfigEntityListBuilder {
    +    $build['#attributes']['id'] = 'field-overview';
    

    Is that really needed anymore ?

  14. +++ b/core/modules/field_ui/src/FieldConfigListBuilder.php
    @@ -32,27 +65,104 @@ class FieldConfigListBuilder extends ConfigEntityListBuilder {
    +    $row = array(
    +      'id' => Html::getClass($field_config->getName()),
    

    Similarly, is the id still needed on each row ?

  15. --- a/core/modules/field_ui/src/FieldOverview.php
    +++ /dev/null
    

    YAY !

  16. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,512 @@
    +  public function buildForm(array $form, FormStateInterface $form_state, $entity_type_id = NULL, $bundle = NULL) {
    ...
    +    $this->entityTypeId = $entity_type_id;
    +    $this->bundle = $form_state->getValue('bundle');
    

    Any way we could avoid those properties ? If those informations are in the $form_state, that should be good enough for the rest of the methods to access them if needed ?

  17. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,512 @@
    +    $form['field'] = array(
    

    So the $form structure is :

    $form['field'] = array(
      '#type' => 'container',
      'type' => ... the "Add new field" select...,
      'separator' => ... the "or" separator ...,
      'existing_field_name' => ... the "Add new field" select...,,
    );
    $form['label'] ...
    

    No real reason why the container is named 'field' ?

    I'd suggest the following :

    $form['add'] = array(
      '#type' => 'container',
      'new_field_type' => ... the "Add new field" select...,
      'separator' => ... the "or" separator ...,
      'existing_storage_name' => ... the "Add new field" select...,,
    );
    
  18. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,512 @@
    +  protected function validateAddNew(array $form, FormStateInterface $form_state) {
    +    $field = $form_state->getValues();
    

    We should get rid of that, it's not a field :-)
    We can name it $values, if we really don't wan't the rest of the code to deal with $form_state ?

  19. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,512 @@
    +      $field_storage = array(
    +        'field_name' => $values['field_name'],
    +        'entity_type' => $this->entityTypeId,
    +        'type' => $values['type'],
    +        'translatable' => $values['translatable'],
    +      );
    +      $field = array(
    +        'field_name' => $values['field_name'],
    +        'entity_type' => $this->entityTypeId,
    +        'bundle' => $this->bundle,
    +        'label' => $values['label'],
    +        // Field translatability should be explicitly enabled by the users.
    +        'translatable' => FALSE,
    +      );
    

    We could inline those in the create() calls, this way we can use $field for "the real FieldConfig that is being created" (instead of current $field is the array of values, and $new_field is the FieldConfig :-)

    Same for the "Add existing" branch

yoroy’s picture

Status: Needs review » Needs work

Whew, thanks yched!

Berdir’s picture

10. This exists 1:1 in HEAD, not sure if we have to touch this here.

amateescu’s picture

Status: Needs work » Needs review
FileSize
94.98 KB
39.07 KB

Awesome review, as always I guess I failed at providing a "nitpick only" patch. Oh well.. maybe next time :)

  1. The problem with providing two label elements as discussed is that we can not make neither of them required :( I'll have to think about this one a bit more.
  2. On the "Add field" page, the last breadcrumb link should be the "Manage fields" page ?
    The breadcrumb trail currently stops one step before that.

    Yup, I agree that we need the last breadcrumb to be "Manage fields", fixed it.

  3. True, but the patch does not make it worse so, not related ?

    Yeah, removed the @todo.

  4. Couldn't this helper function just use the FieldUITestTrait now ?

    Nice spot, fixed.

  5. We probably need to add a RTL version somewhere ? Not sure how this is done these days :-)

    Fixed and found a few more unused styles that needed to be removed as well.

  6. Very unclear that this targets the "or" separating the two selects :-)
    Could we add a specific class to make the selector clearer ?

    Sure, fixed.

  7. Looks like this is lacking some .once() logic, to make sure the event listeners don't get attached several times if behaviors get attached several times.

    Fixed.

  8. Although - at some point we might want to show base fields and / or BaseFieldOverrides there, which wouldn't play nice with the simple "entity list", but well, we'll see when we get there :-)

    Search API is doing something similar in its IndexListBuilder, it lists both indexes and servers in that class because they're displayed on the same screen in the UI, so the pattern is not that unusual :)

  9. Yeah those $this->bundleEntityTypeId are really ugly :-/
    ...
    The phpdoc text is a bit gibberish though :-)

    Made them consistent and improved the description a bit :)

  10. really weird dance with $stored_bundle / $bundle / $form_state['bundle'] - any way we can simplify this ?

    Like @Berdir says, that's just copied over from HEAD. but we can definitely improve it a bit.

  11. That property is not documented as a class member :-)

    Fixed.

  12. Could we at least remove $this->targetBundleEntityType and $this->fieldTypes as properties on the object, and grab the data in the places we really need it ?
    Looks like $this->fieldTypes at least could be easily removed ?

    Yup, got rid of $this->fieldTypes and $this->targetBundleEntityType, but we have to keep the other two :/

  13. Is that really needed anymore ?

    Yes, it's still used in tests for xpath assertions.

  14. Similarly, is the id still needed on each row ?

    As above :)

  15. YAY !

    !!!!

  16. Any way we could avoid those properties ? If those informations are in the $form_state, that should be good enough for the rest of the methods to access them if needed ?

    We could avoid them but then we'd need to pass around $form_state everywhere, not sure that would be cleaner..

    I cleaned up $form['entity_type_id'] and $form['#bundle'] though, because they are definitely duplicating the information from $form_state and hook_form_alter()s can easily use that.

  17. No real reason why the container is named 'field' ?

    That's just the first thing that came to my mind when I wrote the wrapper :)

    And about the change from 'type' to 'new_field_type', I was trying to keep a simple mapping of "form element to field (storage) property name", but I don't really have a strong preference for it, so changed according to your suggestion.

  18. We should get rid of that, it's not a field :-)
    We can name it $values, if we really don't want the rest of the code to deal with $form_state ?

    The code here is much cleaner than it was in FieldOverview so I think it's fine to use $form_state directly :)

  19. We could inline those in the create() calls, this way we can use $field for "the real FieldConfig that is being created" (instead of current $field is the array of values, and $new_field is the FieldConfig :-)

    Sure thing, fixed.

amateescu’s picture

FileSize
105.24 KB
9.01 KB

I found a way to provide two label elements. We just don't use the '#required' property but add the 'form-required' class to the input label and handle the actual validation server-side.

This allowed be to clean up the JS quite a bit and to provide nice validation errors even when the user tries to submit the form without selecting a new field type or an existing field.

amateescu’s picture

FileSize
95.66 KB

Oops, forgot to rebase. Interdiff is good though.

The last submitted patch, 209: 1963340-209.patch, failed testing.

amateescu’s picture

Issue tags: +Ghent DA sprint

New tag needed :)

jibran’s picture

Issue tags: -Ghent DA sprint +Ghent DA sprint. Javascript
FileSize
52.86 KB
68.61 KB
94.53 KB
86.69 KB
57.45 KB
69.57 KB

Add latest screenshots for LTR and RTL field add/error page and manage field page.

plach’s picture

Issue tags: -Ghent DA sprint. Javascript +Ghent DA sprint
jibran’s picture

yched’s picture

@jibran: Thks for the screenshots !

However, to really test LTR, you'd need to check admin/structure/types/manage/page/fields/add-field
With the 'page' node type, you'll get both the "add new" and the "add existing" selects.

swentel’s picture

Screenshot RTL on add field on page content type

yched’s picture

Thanks @amateescu for the last patches. Awesome. Down to mostly nitpicks :-)

  1. +++ b/core/modules/field_ui/field_ui.js
    @@ -3,10 +3,63 @@
    +      var $existingStorageName = $(context).find('select[name="existing_storage_name"]').once('existing-storage-name');
    +      var $fieldLabel = $(context).find('input[name="label"]');
    

    I think it would simplify the code if there was single "once()" on '#field-ui-field-storage-add-form' :

    attach: function (context) {
      var $form = $(context).find('#field-ui-field-storage-add-form').once('field_ui_add');
      if ($form.length) {
        // Do all the attach stuff on our form.
        $form.find('.form-item-label label').addClass('form-required');
        $form.find('select[name="new_field_type"]').change(function () {
          ...
        });
        ...
      }
    };
    
  2. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,524 @@
    +    $form['add_storage_wrapper']['label'] = array(
    ...
    +    $form['add_storage_wrapper']['field_name'] = array(
    

    Uber-nitpick : since the 'field_name' element is used for the first select and the 'label' element is used for the second select, it would be sligntly clearer to define 'field_name' first and 'label' second ?

  3. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,524 @@
    +    $form['add']['new_field_type'] = array(
    +      '#type' => 'select',
    +      '#title' => $this->t('Add a new field'),
    ...
    +      $form['add']['existing_storage_name'] = array(
    +        '#type' => 'select',
    +        '#title' => $this->t('Re-use an existing field'),
    

    Actually, for consistency, 'new_field_type' should be 'new_storage_type' - we're adding a new storage, not a new field.

    My bad, sorry about that :-/

  4. +++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
    @@ -0,0 +1,524 @@
    +    $form['add_storage_wrapper'] = array(
    

    And accordingly, I guess this should be 'new_storage_wrapper', since this is for the 'new storage' select ?

yched’s picture

Crosspost - & thanks @swentel for the last screenshot. Looks great.

amateescu’s picture

FileSize
95.5 KB
13.92 KB

Yay, nitpicks finally :D

  1. I think it would simplify the code if there was single "once()" on '#field-ui-field-storage-add-form' :

    Yes, it does look a bit cleaner. Fixed.

  2. Uber-nitpick : since the 'field_name' element is used for the first select and the 'label' element is used for the second select, it would be sligntly clearer to define 'field_name' first and 'label' second ?

    Nope, that label element is for the first select, it's the label of the new field. The label for "re-use existing" is below $form['new_storage_wrapper'] and it's called $form['existing_storage_label'] :)

  3. Actually, for consistency, 'new_field_type' should be 'new_storage_type' - we're adding a new storage, not a new field.

    My bad, sorry about that :-/

    Fixed. It's just a two-second search and replace, don't lose any sleep over it :P

  4. And accordingly, I guess this should be 'new_storage_wrapper', since this is for the 'new storage' select ?

    Done.

yched’s picture

+++ b/core/modules/field_ui/field_ui.js
@@ -9,16 +9,20 @@
+        $(context).find(
+          '.form-item-label label,' +
...
+        var $newFieldType = $(context).find('select[name="new_storage_type"]');
+        var $existingStorageName = $(context).find('select[name="existing_storage_name"]');

Those can use $form instead of $(context). Less jQ objects to re-create, and a smaller DOM to re-traverse for the .find() selectors. Also, a bit more logical/self-contained: that piece of script is about a specific DOM element (the #field-ui-field-storage-add-form form), and does all its logic on it once/if it finds it.

amateescu’s picture

FileSize
95.49 KB
1.07 KB

True :)

yched’s picture

Thanks ! As far as I'm concerned this is RTBC :-)

Do we need further usability or accessibility validation here ?

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

Nope, its fine. I am going to tentatively mark this RTBC. I ran into some issues with simplytest.me (contact module not installing?) but I think that's not related to this patch.

yched’s picture

Yay ! Three cheers for @amateescu then :-)

yoroy’s picture

Thanks amateescu!

LewisNyman’s picture

Great work Andrei! Thanks

xjm’s picture

Issue summary: View changes
FileSize
55.73 KB
34.78 KB
53.5 KB

The attached are @jibran's screenshots for LTR, cropped down to embed in the summary for reviewers. :)

Looks great! Very happy to see this ready. This is an excellent usability and accessibility improvement.

New button on manage fields

New add field screen

Error message when the field exists

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Wow. Really excellent work on this one! I love that this removes a confusing, one-off interaction in favour of consistent UX patterns that we do everywhere else.

I gave this a pretty thorough working-over and the only part that confused me was why I couldn't add an existing field... until I realized that it's smart and doesn't provide that option when a content type (core Article) already contains all of the available fields (versus adding Image to Page worked fine). HEAD does the same thing, just never noticed it before I guess.

Since this is a UX change, and one that we ideally would've done while D7 was still in development, it should be fine according to https://www.drupal.org/contribute/core/beta-changes. Only problem is it's missing a change record, which is required prior to commit. I worked on one at https://www.drupal.org/node/2393181 but it's still in draft and needs review.

The only other thing I'd love to see at some point is moving away from that awkward middle form where you set # of values, etc. and just stick that on one of the other two (most likely the add form, since it's related to setting up the field storage). But that's not really for this issue to tackle.

So, happily, committed and pushed to 8.0.x. Thanks!

amateescu’s picture

Thanks everyone for all the help and encouragement!

The only other thing I'd love to see at some point is moving away from that awkward middle form where you set # of values, etc. and just stick that on one of the other two (most likely the add form, since it's related to setting up the field storage).

That's the next thing I'm going to tackle. See you in #552604: Adding new fields leads to a confusing "Field settings" form :)

  • webchick committed c02f12b on 8.0.x
    Issue #1963340 by amateescu, dags, andypost, agentrickard, mgifford,...
amateescu’s picture

Assigned: yched » Unassigned

Wrote a couple more things in the "module developers" section of the CR and published it.

nod_’s picture

Follow-up for some JS stuff: #2393391: JS Follow-up to #1963340

Status: Fixed » Closed (fixed)

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

plach’s picture

jibran’s picture