Problem/Motivation

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.
Files: 

Comments

StatusFileSize
new36.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

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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.

@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.

So it will be like adding rules action in D7.

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

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 :-)

Priority:Normal» Major

Upon reflection, I think this is a major.

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).

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

Assigned:Unassigned» dags

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

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...

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).

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

Also over at #1426804: Improve API for module-created fields (with and without instances) 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 ;-)

Issue tags:+Field API

Tagging also

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.

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.

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!

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?

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 :/

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.

StatusFileSize
new66.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

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 :/

@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.

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.

Issue summary:View changes

Updated issue summary.

@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!

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.

Status:Needs work» Active
StatusFileSize
new28.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,974 pass(es), 0 fail(s), and 43 exception(s).
[ View ]

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.

Status:Active» Needs review

Status:Needs review» Needs work

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

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.

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.

@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.

Status:Needs work» Needs review
StatusFileSize
new8.89 KB
FAILED: [[SimpleTest]]: [MySQL] 58,219 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

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.

@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?

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

Status:Needs work» Needs review
StatusFileSize
new28.98 KB
FAILED: [[SimpleTest]]: [MySQL] 57,986 pass(es), 0 fail(s), and 44 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff_27.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new26.19 KB
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]

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.

Status:Needs review» Needs work
StatusFileSize
new64.91 KB
new50.87 KB
new42.03 KB
new54.44 KB
new29.75 KB

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

Step 1:
field_ui-add-field-step-1.pngStep 2:
field_ui-add-field-step-2.1.pngStep 2 (alt):
field_ui-add-field-step-2.2.pngStep 3:
field_ui-add-field-step-3.1.pngStep 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?

Status:Needs work» Needs review
StatusFileSize
new23.37 KB
new48.55 KB
FAILED: [[SimpleTest]]: [MySQL] 57,666 pass(es), 245 fail(s), and 14 exception(s).
[ View ]
  • 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.

Status:Needs work» Needs review
StatusFileSize
new59.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'

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.

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.

@dags Any update?

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

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.

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 ?

StatusFileSize
new35.35 KB
FAILED: [[SimpleTest]]: [MySQL] 57,546 pass(es), 277 fail(s), and 48 exception(s).
[ View ]

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.

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.

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.

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

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

Taking a look at #MWDS.

Status:Needs work» Needs review
StatusFileSize
new31.4 KB
FAILED: [[SimpleTest]]: [MySQL] 57,865 pass(es), 269 fail(s), and 46 exception(s).
[ View ]

Reroll, courtesy of @YesCT.

Status:Needs review» Needs work

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

Will do.

Status:Needs work» Needs review
StatusFileSize
new25.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,850 pass(es), 267 fail(s), and 46 exception(s).
[ View ]

Let's try this one.

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?

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.

Fair enough.

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

Status:Needs work» Needs review
StatusFileSize
new14.49 KB
new38.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,024 pass(es), 86 fail(s), and 20 exception(s).
[ View ]

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.

Assigned:dags» YesCT

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

Assigned:YesCT» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.37 KB
new48.82 KB
FAILED: [[SimpleTest]]: [MySQL] 57,741 pass(es), 87 fail(s), and 21 exception(s).
[ View ]

serenaded tests [edit:] ran.

Status:Needs review» Needs work

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

Issue summary:View changes

Updated issue summary.

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
new49.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,106 pass(es), 102 fail(s), and 20 exception(s).
[ View ]
new221.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.

StatusFileSize
new44.06 KB
FAILED: [[SimpleTest]]: [MySQL] 57,964 pass(es), 214 fail(s), and 63 exception(s).
[ View ]

Straight reroll from #76 before trying to fix tests.

Status:Needs work» Needs review
StatusFileSize
new7.55 KB
new45.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,442 pass(es).
[ View ]

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

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

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

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.

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.

StatusFileSize
new1.56 KB
new45.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,552 pass(es).
[ View ]

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

StatusFileSize
new6.56 KB
new44.6 KB
FAILED: [[SimpleTest]]: [MySQL] 59,065 pass(es), 13 fail(s), and 2 exception(s).
[ View ]

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.

Assigned:Unassigned» Hydra
Status:Needs work» Needs review
StatusFileSize
new45.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,742 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new45.67 KB
PASSED: [[SimpleTest]]: [MySQL] 58,731 pass(es).
[ View ]
new1.38 KB

Okay this should dix the test now.

StatusFileSize
new61.32 KB
new86.62 KB
new62.18 KB
new71.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 ButtonAdd Field ScreenEdit FieldSave Field

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

Status:Needs review» Needs work

Forgot to change the status.

StatusFileSize
new46.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,651 pass(es).
[ View ]
new1.5 KB

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...

Status:Needs work» Needs review

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 ;)

Status:Needs work» Needs review
StatusFileSize
new801 bytes
new46.45 KB
FAILED: [[SimpleTest]]: [MySQL] 58,665 pass(es), 1 fail(s), and 24 exception(s).
[ View ]

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.

StatusFileSize
new94.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

Status:Needs work» Needs review
StatusFileSize
new46.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,683 pass(es).
[ View ]
new875 bytes

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

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.

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.

Should a select element be used for two options?

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.86 KB
new46.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,888 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new47.8 KB
PASSED: [[SimpleTest]]: [MySQL] 59,446 pass(es).
[ View ]
new1.31 KB

Fix the rest

Status:Needs review» Reviewed & tested by the community

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

...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.

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

StatusFileSize
new47.81 KB
FAILED: [[SimpleTest]]: [MySQL] 58,434 pass(es), 14 fail(s), and 3 exception(s).
[ View ]

re-roll

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new59.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,855 pass(es).
[ View ]
new19.63 KB

fixed patch:
- use FormBase
- title set on route

StatusFileSize
new59.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.field-ui.module.1963340-112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.34 KB
new17.67 KB

Minor clean-up. Still needs work.

field_ui-add.png

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

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: Add the machine name component to the Seven Style guide

Screen Shot 2013-10-09 at 11.34.33.jpgScreen 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

+++ 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.

Issue summary:View changes

Removed totally unrelated issue.

Status:Needs review» Needs work

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

Assigned:Hydra» yched
Issue summary:View changes

Lets have yched take a look at this.

Assigned:yched» amateescu

Working on this.

Status:Needs work» Needs review
Issue tags:+LONDON_2013_DECEMBER
StatusFileSize
new89.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,690 pass(es), 359 fail(s), and 170 exception(s).
[ View ]
new74.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..

#120 is a tremendous clean-up and code decoupling that will allow to simplify #2094499: "Manage (form) display" forms should directly receive an EntityDisplay object

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

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 :)

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.

StatusFileSize
new88.62 KB
FAILED: [[SimpleTest]]: [MySQL] 58,731 pass(es), 359 fail(s), and 168 exception(s).
[ View ]
new11.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.

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,424 pass(es).
[ View ]
new786 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? :)

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

StatusFileSize
new88.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.

StatusFileSize
new90.38 KB

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

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

StatusFileSize
new90.56 KB
new7.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.

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

Yep, #132 fixes that error.

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.

StatusFileSize
new90.52 KB
new1.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.

Issue tags:+DrupalWorkParty
StatusFileSize
new91.71 KB

StatusFileSize
new84.23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Re-roll.

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

@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.

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 :)

Status:Needs work» Needs review
StatusFileSize
new91.92 KB
new3.38 KB

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

StatusFileSize
new91.94 KB
new10.65 KB

Missed some core changes.

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.

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.

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.

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