Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
- The current field creation workflow in the field UI is somewhat overwhelming to users.
- Users have to step through a series of three forms to add a field.
- Additionally, there are some accessibility concerns with having the "Add new field" and "Reuse existing field" options in the field listing table.
- In #1847590: Fix UX of entity reference field type selection and #1847596: Remove Taxonomy term reference field in favor of Entity reference, @yched (the field maintainer) has suggested moving the "Add field" operation onto its own form so that there is more space to provide a good interaction for creating a new field.
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 |
|
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:
- Enters the field label, which automatically generates a machine name as per usual.
- 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
- Go to content types -> basic page -> Manage fields
- Add a field
UI changes
New button on manage fields
New add field screen
Error message when the field exists
Comment | File | Size | Author |
---|---|---|---|
#228 | Add-Field-Error-en.png | 53.5 KB | xjm |
#228 | Add-Field-en.png | 34.78 KB | xjm |
#228 | Manage-Field-en.png | 55.73 KB | xjm |
#222 | interdiff.txt | 1.07 KB | amateescu |
#222 | 1963340-222.patch | 95.49 KB | amateescu |
Comments
Comment #1
xjmNo idea what is going on with attachments to issues this week. This is a mockup of the manage fields screen.
Comment #1.0
xjmUpdated issue summary.
Comment #1.1
xjmUpdated issue summary.
Comment #2
tim.plunkettThe 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.
Comment #3
xjm@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.
Comment #4
jibranSo it will be like adding rules action in D7.
Comment #5
amateescu CreditAttribution: amateescu commentedRe #3: This was discussed in another issue and Bojhan was opposed to the idea: #1855992-8: Use dialog api for formatter settings.
Comment #6
yched CreditAttribution: yched commentedTrue, 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 :-)
Comment #7
xjmUpon reflection, I think this is a major.
Comment #8
yched CreditAttribution: yched commentedAlso, 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).
Comment #9
Bojhan CreditAttribution: Bojhan commentedI would support this change :) It will be a small gain, but a considerable one when you have loads of distracting elements on the page
Comment #10
dags CreditAttribution: dags commentedxjm coaxed me into this one. ;) ...working on an initial patch.
Comment #11
falcon03 CreditAttribution: falcon03 commentedI 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...
Comment #12
swentel CreditAttribution: swentel commentedBojhan 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).
Comment #13
falcon03 CreditAttribution: falcon03 commented@swentel: thanks for clarifying. I would like having them in separate forms as well: it's always better than the current UI, IMO.
Comment #14
dags CreditAttribution: dags commentedWaiting for #1946404: Convert forms in field_ui.admin.inc to the new form interface to land before resuming work.
Comment #15
swentel CreditAttribution: swentel commentedAlso 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 ;-)
Comment #16
swentel CreditAttribution: swentel commentedTagging also
Comment #17
ainz CreditAttribution: ainz commentedJust 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.
Comment #18
tim.plunkettThose 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.
Comment #19
klonosPersonally 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!
Comment #20
dags CreditAttribution: dags commentedI 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?
Comment #21
klonosI 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 :/
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #23
ainz CreditAttribution: ainz commentedI 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.
Comment #24
klonosI'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 :/Comment #25
ainz CreditAttribution: ainz commented@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.
Comment #26
tim.plunkett#1946404: Convert forms in field_ui.admin.inc to the new form interface is in now!
Comment #27
dags CreditAttribution: dags commentedStill 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.
Comment #27.0
dags CreditAttribution: dags commentedUpdated issue summary.
Comment #28
xjm@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!
Comment #29
swentel CreditAttribution: swentel commentedAlso 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.
Comment #30
dags CreditAttribution: dags commentedOk 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.
Comment #31
xjmComment #33
dags CreditAttribution: dags commentedI'm following #1969728: Implement Field API "field types" as TypedData Plugins and am working a new patch based on those changes.
Comment #34
dags CreditAttribution: dags commentedIn 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:
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.
Comment #35
xjm@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.
Comment #37
mgiffordre-roll mostly for element-invisible swap.
Comment #39
dags CreditAttribution: dags commented@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?
Comment #40
xjm@dags, when I have a working patch, I usually just take screenshots and annotate them with skitch.
Comment #41
mgiffordSeems 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/
Comment #43
dags CreditAttribution: dags commented@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.
Comment #44
dags CreditAttribution: dags commentedFieldAddForm 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.
Comment #46
dags CreditAttribution: dags commentedThe patch in #44 creates a new FieldAddForm that renders like this:
Step 1:
Step 2:
Step 2 (alt):
Step 3:
Step 3 (alt):
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?
Comment #47
dags CreditAttribution: dags commentedNow the FieldOverview form looks like xjm's mockup in comment #1. Submitting it updates field weights by calling $entity_from_display->save().
Comment #49
tim.plunkettThis 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
The "field_" prefix is now configurable. It doesn't look like this was fixed as part of that issue, but keep that in mind.
Why is this needed here? Can't we do this with hook_local_actions() instead?
Are these really better as static?
These are all wrappers around OO code, let's use the OO bits and possibly inject what we can.
Same as before, this is injectable. Use the OO code it wraps
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'
Comment #50
dags CreditAttribution: dags commentedThanks 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.
Comment #51
tim.plunkettThe -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.
Comment #52
YesCT CreditAttribution: YesCT commented@dags Any update?
Comment #53
dags CreditAttribution: dags commentedI 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
Comment #54
YesCT CreditAttribution: YesCT commentedThis 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.
Comment #55
yched CreditAttribution: yched commentedGetting #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 ?
Comment #56
dags CreditAttribution: dags commentedWhew. 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.
Comment #57
amateescu CreditAttribution: amateescu commentedThanks for staying on top of all these changes! :)
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.
Comment #58
falcon03 CreditAttribution: falcon03 commentedGlad 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.
Comment #60
xjmThanks @falcon03! Let's make sure to do a full accessibility review on the patch once it is ready.
Comment #61
falcon03 CreditAttribution: falcon03 commented@xjm: sure. I'll do a full accessibility review as soon as the patch is ready. Honestly, I'm looking forward to doing it! ;)
Comment #62
agentrickardTaking a look at #MWDS.
Comment #63
agentrickardReroll, courtesy of @YesCT.
Comment #64
amateescu CreditAttribution: amateescu commentedThe JS added in this patch needs to be removed, along with the widget selector. See #57 :)
Comment #65
agentrickardWill do.
Comment #66
agentrickardLet's try this one.
Comment #67
agentrickardI do find it confusing that I cannot select the widget type from the Add/Edit workflow. Are we planning to add that back in?
Comment #68
amateescu CreditAttribution: amateescu commentedFor 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.
Comment #70
agentrickardFair enough.
All these test fails are expected, and I am working my way through them now.
Comment #71
agentrickardFixed 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.
Comment #73
YesCT CreditAttribution: YesCT commentedI'm working on fixing tests. patch to come shortly.
Comment #74
YesCT CreditAttribution: YesCT commentedserenaded tests [edit:] ran.
Comment #75.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary.
Comment #76
YesCT CreditAttribution: YesCT commentedI 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
The entity type is coming up node, and there is no contact rrrr node type. So the "Add field" link url is wrong.
@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.
Comment #78
agentrickardStraight reroll from #76 before trying to fix tests.
Comment #79
agentrickardAnd this test should work. I simplified the click-behavior testing of the contact form, which really isn't necessary.
Comment #80
tim.plunkettI remember that test coverage being explicitly added. clickLink() helps us ensure the UI actually works, not just the forms with drupalPost().
Comment #81
BerdirYes, everyone wants to remove my test ;) We should add a comment and explain why it's there.
Comment #82
agentrickardThat 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.
Comment #83
BerdirIt 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.
Comment #84
agentrickardI'm not willing to argue about those tests in this patch. Restored.
Comment #85
swentel CreditAttribution: swentel commentedReroll after entity storage and some other patches got in. Not entirely sure whether it will pass, but we'll see.
Comment #87
Hydra CreditAttribution: Hydra commentedI tried creating an interdiff, but I "accidentally" fixed a lot of things when I was manually rerolling the patch.. :/
Comment #89
Hydra CreditAttribution: Hydra commentedOkay this should dix the test now.
Comment #90
mgiffordSeems 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.
It's this last screen that's the problem.
Comment #91
mgiffordForgot to change the status.
Comment #92
pguillard CreditAttribution: pguillard commentedHere 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...
Comment #93
pguillard CreditAttribution: pguillard commentedComment #94
rteijeiro CreditAttribution: rteijeiro commentedThis should not be part of the patch ;)
Comment #95
pguillard CreditAttribution: pguillard commentedThis time should be correct with a correct interdiff. Sorry.
Comment #97
rteijeiro CreditAttribution: rteijeiro commentedAfter 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).
Comment #98
pguillard CreditAttribution: pguillard commentedI did a bad mistake rerolling my patch, copy pasting wrong line. This one should be ok now!
Comment #99
rteijeiro CreditAttribution: rteijeiro commentedPatch #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.
Comment #100
dags CreditAttribution: dags commentedI 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.
Comment #101
Grayside CreditAttribution: Grayside commentedShould a select element be used for two options?
Comment #102
alexpottPatch no longer applies.
Comment #103
andypostRe-roll
Also label should be populated with
$this->randomString()
Comment #105
andypostFix the rest
Comment #106
agentrickardSmall changes in the re-roll. Should still be RTBC.
Comment #107
klonos...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.
Comment #108
andypostAlso it could be good UX to use list of field types (as node/add does) when there's less then 10-15 field types
Comment #109
andypostre-roll
Comment #111
andypostfixed patch:
- use
FormBase
- title set on route
Comment #112
andypostMinor clean-up. Still needs work.
Comment #113
andypostAlso '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/1736366Comment #114
LewisNymanThis 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
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”?
Comment #115
XanoNow 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.
Comment #115.0
XanoRemoved totally unrelated issue.
Comment #116
mgifford112: drupal8.field-ui.module.1963340-112.patch queued for re-testing.
Comment #118
Bojhan CreditAttribution: Bojhan commentedLets have yched take a look at this.
Comment #119
amateescu CreditAttribution: amateescu commentedWorking on this.
Comment #120
amateescu CreditAttribution: amateescu commentedHere'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 :)
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..
Comment #121
andypost#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
Comment #122
amateescu CreditAttribution: amateescu commentedMy 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."
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 :)
Comment #123
yched CreditAttribution: yched commentedw00t! ++
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 :-)
Comment #125
amateescu CreditAttribution: amateescu commentedApparently, the #states approach doesn't work very well so I switched the field type select to #ajax instead.
Comment #127
amateescu CreditAttribution: amateescu commentedUpdated 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? :)
Comment #128
Bojhan CreditAttribution: Bojhan commentedIs this truly ready? I have RTBC patches too much lately, that ended up not working well.
Comment #129
amateescu CreditAttribution: amateescu commented@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.
Comment #130
amateescu CreditAttribution: amateescu commentedRerolled. Is there any chance to get a UX review here or we should carry on with code review and test fixing?
Comment #131
Bojhan CreditAttribution: Bojhan commentedThis patch doesn't apply and creates a error on initiation?
Comment #132
amateescu CreditAttribution: amateescu commentedYou'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.
Comment #133
mgiffordI 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
Comment #134
amateescu CreditAttribution: amateescu commentedYep, #132 fixes that error.
Comment #135
Bojhan CreditAttribution: Bojhan commentedIt 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.
Comment #136
amateescu CreditAttribution: amateescu commentedThanks 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.
Comment #137
amateescu CreditAttribution: amateescu commentedRerolled after #2141329: Replace getAdminPath() with getAdminRouteInfo() in Field UI.
Comment #138
XanoRe-roll.
@amateescu: I assume the sprint tags can be removed?
Comment #139
amateescu CreditAttribution: amateescu commented@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.
Comment #141
andypostJust 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"
Comment #142
amateescu CreditAttribution: amateescu commentedThanks @andypost, this means we need more usability testing here. In the meantime, here's a proper reroll.
Comment #143
amateescu CreditAttribution: amateescu commentedMissed some core changes.
Comment #144
swentel CreditAttribution: swentel commentedBerdir 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.
Comment #145
yched CreditAttribution: yched commentedNot 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.
Comment #146
BerdirMy 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.
Comment #147
yched CreditAttribution: yched commented#146: ok, right - so, not strictly related to Instances & #145, and true, the current is less than ideal :-/.
Comment #148
star-szrBump, is this still a viable path? Was just re-checking #1938900: Convert theme_field_ui_table into a template.
Comment #149
amateescu CreditAttribution: amateescu commentedI guess it is, but the patch has to be rewritten mostly from scratch, too many things changed in these 8 months.
Comment #150
xjmYeah, it would be great if we could still do this. It will definitely take some work to get an updated patch.
Comment #151
BerdirThis 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?
Comment #152
webchickWe 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.
Comment #153
andypostTalked 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 wayComment #154
Bojhan CreditAttribution: Bojhan commented@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.
Comment #155
amateescu CreditAttribution: amateescu commentedRC 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.
Comment #156
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #157
mgiffordThanks @amateescu - looks like you've moved it ahead a ways.
For the next 5 hours there's an instance up on simplytest.me
Comment #158
LewisNymanComment #159
amateescu CreditAttribution: amateescu commentedDone :)
Comment #161
LewisNymanThis 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.
Comment #162
amateescu CreditAttribution: amateescu commentedThanks Lewis! Easy fix for #161.
Comment #163
LewisNymanI wonder if we can use container-inline for this instead of new CSS?
Comment #164
amateescu CreditAttribution: amateescu commentedI tried that initially but container-inline also puts the labels inline and it doesn't look that good anymore :/
Comment #165
Bojhan CreditAttribution: Bojhan commentedLets go ahead and fix this issue mentioned by lewis, it seems like we are on track to get this in!
Comment #166
amateescu CreditAttribution: amateescu commentedOk then, since we have approval from Lewis and Bojhan for the current approach, I'll go ahead and start fixing tests (yay...).
Comment #167
xjmEvaluating this in terms of the allowed beta changes policy (https://www.drupal.org/core/beta-changes):
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!
Comment #168
xjmComment #169
xjmComment #170
amateescu CreditAttribution: amateescu commented@xjm, cool! Here's a green patch to go along with that evaluation.
Comment #171
yched CreditAttribution: yched commentedThanks 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) ?
Comment #172
yched CreditAttribution: yched commentedThe 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 :-)
Comment #173
Bojhan CreditAttribution: Bojhan commentedWhat 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.
Comment #174
amateescu CreditAttribution: amateescu commentedThis 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.
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.
Sigh.. I was hoping this easter egg would be discovered much later :)
Comment #175
amateescu CreditAttribution: amateescu commentedOops, 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.
Comment #176
yched CreditAttribution: yched commentedYes, agreed with the drawbacks of "2 separate entry points" :
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 ?
Comment #177
amateescu CreditAttribution: amateescu commentedRe #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.
Comment #178
yched CreditAttribution: yched commentedGood question - I'd think yes, given that this is the most frequent use case ?
Good question too - I don't know about that though. Maybe others know ? @Bojhan ?
Comment #179
Bojhan CreditAttribution: Bojhan commentedThere should be no accessibility issues with it, I am quite sure of that.
Comment #180
amateescu CreditAttribution: amateescu commentedAre there any other opinions on the approach suggested in #176 before I start implementing it?
Comment #181
amateescu CreditAttribution: amateescu commentedDiscussed 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.
Comment #182
yoroy CreditAttribution: yoroy commentedApproach in #176 sounds great :)
Comment #183
yoroy CreditAttribution: yoroy commentedha, posted at almost the same time :)
Comment #185
amateescu CreditAttribution: amateescu commentedI 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.
Comment #186
amateescu CreditAttribution: amateescu commentedThat 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 :)
Comment #187
yoroy CreditAttribution: yoroy commentedThis 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" :-)
Comment #188
yoroy CreditAttribution: yoroy commentedWhen 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.
Comment #189
Wim LeersIt 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:
Comment #190
BerdirYes, 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.
Comment #191
yoroy CreditAttribution: yoroy commentedThanks, 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?
Comment #192
BerdirIt 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.
Comment #193
yoroy CreditAttribution: yoroy commentedThanks 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!
Comment #194
amateescu CreditAttribution: amateescu commented@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?
Comment #195
yoroy CreditAttribution: yoroy commentedFirst 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.
Comment #196
amateescu CreditAttribution: amateescu commented@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 :)
Comment #197
LewisNymanhmm 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.
Comment #198
yoroy CreditAttribution: yoroy commentedHa, 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.
Comment #199
amateescu CreditAttribution: amateescu commentedMy 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?
Comment #200
yoroy CreditAttribution: yoroy commentedYes! :)
Comment #201
amateescu CreditAttribution: amateescu commentedHere 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.
Comment #202
amateescu CreditAttribution: amateescu commentedWell.. 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.
Comment #203
yoroy CreditAttribution: yoroy commentedDoing the not-so-subtle thing and assigning to yched for review :)
Comment #204
swentel CreditAttribution: swentel commentedWent 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 :)
Comment #205
yched CreditAttribution: yched commentedYay, patch is full of awesome, thanks a ton @amateescu !
I do have remarks, but nothing big.
The breadcrumb trail currently stops one step before that.
True, but the patch does not make it worse so, not related ?
Couldn't this helper function just use the FieldUITestTrait now ?
We probably need to add a RTL version somewhere ? Not sure how this is done these days :-)
Very unclear that this targets the "or" separating the two selects :-)
Could we add a specific class to make the selector clearer ?
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.
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 :-)
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 :-)
really weird dance with $stored_bundle / $bundle / $form_state['bundle'] - any way we can simplify this ?
That property is not documented as a class member :-)
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 ?
Is that really needed anymore ?
Similarly, is the id still needed on each row ?
YAY !
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 ?
So the $form structure is :
No real reason why the container is named 'field' ?
I'd suggest the following :
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 ?
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
Comment #206
yoroy CreditAttribution: yoroy commentedWhew, thanks yched!
Comment #207
Berdir10. This exists 1:1 in HEAD, not sure if we have to touch this here.
Comment #208
amateescu CreditAttribution: amateescu commentedAwesome review, as always I guess I failed at providing a "nitpick only" patch. Oh well.. maybe next time :)
Yup, I agree that we need the last breadcrumb to be "Manage fields", fixed it.
Yeah, removed the @todo.
Nice spot, fixed.
Fixed and found a few more unused styles that needed to be removed as well.
Sure, fixed.
Fixed.
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 :)
Made them consistent and improved the description a bit :)
Like @Berdir says, that's just copied over from HEAD. but we can definitely improve it a bit.
Fixed.
Yup, got rid of $this->fieldTypes and $this->targetBundleEntityType, but we have to keep the other two :/
Yes, it's still used in tests for xpath assertions.
As above :)
!!!!
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.
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.
The code here is much cleaner than it was in FieldOverview so I think it's fine to use $form_state directly :)
Sure thing, fixed.
Comment #209
amateescu CreditAttribution: amateescu commentedI 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.
Comment #210
amateescu CreditAttribution: amateescu commentedOops, forgot to rebase. Interdiff is good though.
Comment #212
amateescu CreditAttribution: amateescu commentedNew tag needed :)
Comment #213
jibranAdd latest screenshots for LTR and RTL field add/error page and manage field page.
Comment #214
plachComment #215
jibranTag correction :/
Comment #216
yched CreditAttribution: yched commented@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.
Comment #217
swentel CreditAttribution: swentel commentedScreenshot RTL on add field on page content type
Comment #218
yched CreditAttribution: yched commentedThanks @amateescu for the last patches. Awesome. Down to mostly nitpicks :-)
I think it would simplify the code if there was single "once()" on '#field-ui-field-storage-add-form' :
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 ?
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 :-/
And accordingly, I guess this should be 'new_storage_wrapper', since this is for the 'new storage' select ?
Comment #219
yched CreditAttribution: yched commentedCrosspost - & thanks @swentel for the last screenshot. Looks great.
Comment #220
amateescu CreditAttribution: amateescu commentedYay, nitpicks finally :D
Yes, it does look a bit cleaner. Fixed.
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']
:)Fixed. It's just a two-second search and replace, don't lose any sleep over it :P
Done.
Comment #221
yched CreditAttribution: yched commentedThose 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.
Comment #222
amateescu CreditAttribution: amateescu commentedTrue :)
Comment #223
yched CreditAttribution: yched commentedThanks ! As far as I'm concerned this is RTBC :-)
Do we need further usability or accessibility validation here ?
Comment #224
Bojhan CreditAttribution: Bojhan commentedNope, 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.
Comment #225
yched CreditAttribution: yched commentedYay ! Three cheers for @amateescu then :-)
Comment #226
yoroy CreditAttribution: yoroy commentedThanks amateescu!
Comment #227
LewisNymanGreat work Andrei! Thanks
Comment #228
xjmThe 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
Comment #229
xjmThis also blocks further usability work for #1847596: Remove Taxonomy term reference field in favor of Entity reference and #1847590: Fix UX of entity reference field type selection.
Comment #230
webchickWow. 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!
Comment #231
amateescu CreditAttribution: amateescu commentedThanks everyone for all the help and encouragement!
That's the next thing I'm going to tackle. See you in #552604: Adding new fields leads to a confusing "Field settings" form :)
Comment #233
amateescu CreditAttribution: amateescu commentedWrote a couple more things in the "module developers" section of the CR and published it.
Comment #234
nod_Follow-up for some JS stuff: #2393391: JS Follow-up to #1963340
Comment #236
plachRelated issue: #2401497: Field UI creates fields that can never be translated.
Comment #237
jibranThere is an other change notice in draft queue https://www.drupal.org/node/2404475 which is not relevant anymore because of #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects so I suggest we update the 2404475 to accommodate the changes of #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects. Or we can just delete it.