Problem/Motivation

This is a breakout issue from #516138-139: Move CCK HEAD in core. The first screen upon adding a field is confusing, given that you are sent into field settings which may or may not have anything editable, and then still need to complete another screen. Steps to reproduce:

- add a field to the article content type
- you got redirected to a page with settings for the field
- BUT the field may not have global settings, and even though we are apparently under the Article content type (see page title), Article specific settings are not displayed either
- go to manage fields again and to edit the particular field and now you get both Article specific settings and actual global settings (which in the previous screen were said to be nonexistent)

Proposed resolution

Display field settings right away:

Remaining tasks

Ensure we want to do this. Fix tests. Fix any outstanding problems.

User interface changes

See proposed resolution.

API changes

Maybe, depending on extent of UI change needed.

Data model changes

None.

CommentFileSizeAuthor
#155 552604-155.patch32.65 KBPancho
#155 552604_153-155_interdiff.txt8.52 KBPancho
#153 552604-153.patch25.34 KBPancho
#139 interdiff.txt839 bytesamateescu
#139 552604-139.patch38.97 KBamateescu
#137 interdiff.txt8.43 KBamateescu
#137 552604-137.patch39.02 KBamateescu
#132 interdiff.txt8.75 KBamateescu
#132 552604-132.patch31.82 KBamateescu
#124 Manage fields | drupal 8.0.x 2016-02-24 16-40-08.png118.47 KBGábor Hojtsy
#124 Decimal settings for Article | drupal 8.0.x 2016-02-24 16-38-14.png111.48 KBGábor Hojtsy
#124 Add field | drupal 8.0.x 2016-02-24 16-30-22.png93.9 KBGábor Hojtsy
#122 552604-122.patch23.95 KBGábor Hojtsy
#121 interdiff.txt14.86 KBamateescu
#121 552604-121-do-not-test.patch23.95 KBamateescu
#119 interdiff.txt12.75 KBamateescu
#119 552604-119-do-not-test.patch21.47 KBamateescu
#117 552604-do-not-test.patch29.2 KBamateescu
#99 552604-99.patch30.16 KBswentel
#97 confusing-s00a-nopatch-editfield-New Thing | confusing-552604-92 2012-11-27 10-35-42.png85.07 KBYesCT
#97 confusing-s00b-fieldsettings-New Thing | confusing-552604-92 2012-11-27 10-37-03.png47.64 KBYesCT
#97 confusing-s01-add-field-manage-fields-2012-11-27_1004.png129.41 KBYesCT
#97 confusing-s02-after-save-global-field-settings-2012-11-27_1006.png105 KBYesCT
#97 confusing-s03-non-global-settings-edit-New Thing settings for Article | confusing-552604-92 2012-11-27 10-09-10.png58.64 KBYesCT
#97 confusing-s04-onenode-hascontent-2012-11-27_1010.png53.61 KBYesCT
#97 confusing-s05-managefields-2012-11-27_1013.png114.34 KBYesCT
#97 confusing-s06-editfieldwithcontentexisting-New Thing settings for Article | confusing-552604-92 2012-11-27 10-14-56.png58.05 KBYesCT
#97 confusing-s07-fieldsettingswithcontentexisting-2012-11-27_1016.png103.83 KBYesCT
#92 552604-92.patch23.94 KBswentel
#83 drupal8.field-instance-settings.83.patch23.66 KBsun
#81 drupal8.field-instance-settings.81.patch20.52 KBsun
#73 drupal8.field-instance-settings.73.patch18.03 KBsun
#73 interdiff.txt6.64 KBsun
#72 interdiff.txt5.64 KBsun
#66 552604-field-ui-65.patch21.58 KBLukas von Blarer
#66 552604-field-ui-interdiff-65.txt1.62 KBLukas von Blarer
#66 Screen Shot 2012-11-06 at 4.35.53 PM.png19.24 KBLukas von Blarer
#66 Screen Shot 2012-11-06 at 4.36.12 PM.png51.9 KBLukas von Blarer
#66 Screen Shot 2012-11-06 at 4.37.08 PM.png26.49 KBLukas von Blarer
#66 Screen Shot 2012-11-06 at 4.37.39 PM.png61.07 KBLukas von Blarer
#66 Screen Shot 2012-11-06 at 4.36.32 PM.png48.24 KBLukas von Blarer
#62 drupal8.field-instance-settings.62.patch17.13 KBsun
#45 drupal.field-instance-settings.45.patch19.08 KBsun
#44 list-field-settings.png22.24 KBsun
#44 list-instance-settings.png21.02 KBsun
#44 text-field-settings.png20.75 KBsun
#44 text-instance-settings.png26.1 KBsun
#43 drupal.field-instance-settings.43.patch17.83 KBsun
#41 field_ui_instance_settings_form-552604-41.patch12.27 KByched
#39 drupal.field-instance-settings.39.patch7.93 KBsun
#29 drupal.field-instance-settings.29.patch7.02 KBsun
#28 drupal.field-instance-settings.28.patch6.34 KBsun
#12 first_page.png16.32 KBlambic
#12 second_page.png16.38 KBlambic
#9 552604.patch1.14 KBlambic
#5 552604.patch1.08 KBlambic
#2 552604.patch1.41 KBlambic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Component: field system » field_ui.module
lambic’s picture

FileSize
1.41 KB

I'm attaching a patch to fix the title issue, but ideally the first screen should be skipped completely when there are no applicable settings.

lambic’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

lambic’s picture

FileSize
1.08 KB

rerolled patch

lambic’s picture

Status: Needs work » Needs review
Bojhan’s picture

Sorry, but requires screenshot

Dave Reid’s picture

Let's also remove that check_plain() w/ drupal_set_title() since it would be run through that function twice.

lambic’s picture

FileSize
1.14 KB

check_plain() removed.

Bojhan’s picture

@lambic How can you not make a screenshot, with check_plain() removed? It should be beautiful now :P

yched’s picture

+  $title = isset($instance['title']) ? $instance['title'] : $instance['label'];

I might be missing something, but I don't see where $instance['title'] would come from ?

lambic’s picture

FileSize
16.38 KB
16.32 KB

finally got around to screenshots:

lisarex’s picture

Applied the patch in #9 and this a definite improvement.

MichaelCole’s picture

Issue tags: -Usability, -Fields in Core

#9: 552604.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +Fields in Core

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

webchick’s picture

Priority: Normal » Major

Back looking at this workflow again after awhile, I really think this is a bad enough problem we ought to see if we can do something to smooth it out before release. What, I don't really know, since it's quite late. But while it might make sense to Field API folks why adding a field results in two settings screens shown right after another, the second with exactly the same options as the first (only at the bottom this time for extra pizazz!), I can assure you that no one else on the entire planet will understand why this happens.

Over at #867968: Field UI settings workflow is dumb, Bojhan writes:

Trying to add some clarity to this discussion on actual steps that need to be taken.

1. Correct indicators, where you are in the process of creating a field.
2. Correct labels moving between pages, ie.. "Save and continue" , "Create field"
3. The actual widget settings page is not designed, but pulled together there is no coherence to its hierarchy and ignores most Drupal core UX standards.
4. There are two setting pages [postpone - unless we can easily get it done]

I would suggest if you want to actually move this before we release Drupal 7 is to create individual issues for example those outlined above.

To me, #4 - the fact that there are two settings pages, with no remotely obvious difference between the two - is the biggest UX wtf here. I do not understand why the first one is even needed, since presumably if I navigated away from the first settings form without saving it, my entire site would not explode, which means that Field API must be doing something smart under the hood in terms of defaults. But the rest of those sound good, too. Separate issues might make sense, but let's come up with a plan first.

Can someone remind me again why we have two settings pages in the first place, and can't just immediately show the widget configuration form assuming the defaults from the field?

yched’s picture

yched’s picture

Part of the issue is #821390: Fields usable before all steps to create them are completed. : The forms you go through when creating a field are in fact just the edit forms for the field and the instance. You're editing a field that has been created with default settings and properties the moment you pressed 'Save' on the first screen ('Manage fields')

webchick’s picture

Yep, agreed there on the edit form thing. So forcing users through this workflow is really awkward, as we don't do this elsewhere in core (congratulations, you just created something! here, now edit its settings. and now edit MORE of its settings!). I would say just ditch the redirection to the settings forms altogether except for the fact that it's not at all clear that clicking a link titled "Autocomplete term widget (tagging)" will take you to a settings form, so I'm not sure how someone would ever find their way back there (actually, that's true even now).

What was the reason again that we can't just stick all of the settings on admin/structure/types/manage/article/fields/field_tags like it was with CCK, instead of having 3 separate settings pages? Then that at least would mesh better with our existing UX elsewhere, which is "to change something's settings, click the edit link next to it".

yched’s picture

- separate form to change the widget type:
Because the current widget has an effect on the content of the instance settings - back then #ajax was not where it is today.
This being said, I've been thinking lately that we could let users pick and change the widget with a select dropdown straight from the 'Manage fields' table, rather than linking to a separate form that has a single select.

- separate form for field settings:
Because in D6 everybody screamed that the one-form-including-field-and-instance-settings was too damn big and scary.
Also, in D7, with storage abstraction niceties, some *field* settings (the ones that would affect the schema) cannot be changed after the field creation. While you can change anything at the instance level (doesn't affect schema anyway).
This being said, when doing the initial port of D7 CCK in contrib, Karen left a copy of the 'field' settings form at the bottom of the instance settings form, and this was left as is in #516138: Move CCK HEAD in core...
I've been willing to write a patch to remove that bottom 'field settings' fieldset for weeks :-/.

Also of note:
The 'instance settings' form actually contains
- widget settings, that only affect the how the field is edited, and which can vary depending on the selected widget
- and instance settings : properties of the field that can vary from a content type to the other (required, label, allowed file extensions...), that will be there no matter what the widget)
Those two garbled and intertwined in the same fieldset.
While working on #516138: Move CCK HEAD in core, I advocated for a clearer separation of the two, but quicksketch voiced against.

webchick’s picture

Cool, thanks for the brain-dump, yched. I'll do some thinkering.

Anyone else, feel free to chime in with suggestions, too.

yched’s picture

Thx webchick, much appreciated :-)

sun’s picture

Issue tags: +Needs usability review

Probably not of much help here, but I think:

+1 for clearly separating field from instance from widget settings. If there's a technical design concept behind it, then it doesn't make sense to try to hide that concept in the UI. First source of confusion.

+1 for either directly jumping to instance settings or back to "Manage fields" overview page after creation. The field settings page is the least interesting, and changing configuration options most probably is a rare action anyway, as you're performing site-wide changes to the field.

In general, I think that we badly need to clarify the on-screen tips and descriptions, so as to make users understand when they ought to re-use an existing field, and in which common cases it isn't really a good idea. I'm struggling myself in trying to explain it.

+1 for more #ajax. However, given that the other ajaxified field formatter settings patch was relatively simple, we would have to start with "ideal world mockups" here.

sun.core’s picture

Issue tags: +String freeze, +API change

Can we distill what's still possible to do for D7? Some of the sub-issues raised here are indeed major, some others are too late for D7.

naught101’s picture

subscribe

sun’s picture

Status: Needs work » Needs review
FileSize
6.34 KB

Remove confusion.

sun’s picture

Additionally resolving:

- you got redirected to a page with settings for the field
- BUT the field has no global settings, and even though we are apparently under the Article content type (see page title), Article specific settings are not displayed either

sun’s picture

Assigned: Unassigned » sun

Status: Needs review » Needs work

The last submitted patch, drupal.field-instance-settings.29.patch, failed testing.

webchick’s picture

Much as I would love to say otherwise, I honestly don't think we can do anything about this in D7 now. We're post-RC. :(

sun’s picture

I'd agree, if it wouldn't be technically, fundamentally wrong to expose the field settings form on the instance settings form. Most field settings of most fields cannot be (and must not be) changed, so exposing the field settings on that form in the first place makes absolutely no sense at all.

...not to mention that, during debugging of the List field patch just recently, @grendzy reported that you *can* change field settings on the instance settings form without getting a form error... (rather off-topic - actually needs to be a separate issue, because, regardless of where the field settings are getting exposed, we need to ensure that they cannot be changed)

webchick’s picture

Is the patch you have uploaded reviewable? (it's failing tests but not sure if those are academic failures or horrible problems failures.) I'd need to evaluate the UI impact to make a call on this, I think. If the patch isn't reviewable, I'd be fine with just a general old vs. new workflow description, too.

yched’s picture

(if webchick approves this, I'm all for it - thanks sun !)

sun’s picture

The patch works and can be applied. The test failures only seem to be caused by test code that expects the field settings on the instance settings form, so fixing those should be relatively easy.

sun’s picture

@yched: Manual testing of this patch reveals that the field settings on the instance settings form currently contain the "Number of values" configuration option... which is not contained in the dedicated field settings form... so does that belong into field settings or instance settings? ;)

yched’s picture

Cardinality is a field-level setting. it does belong in the 'field settings' form.

sun’s picture

Status: Needs work » Needs review
FileSize
7.93 KB

Alright, so:

1) Moved the "Maximum number of values" setting that was previously hard-coded into the instance settings form into the field settings form, where it's supposed to be.

2) Due to 1), now it makes sense to always show the field settings form as first step after creating a new field, even when the field itself has no settings. Thus, reverted that change from the previous patch and added an inline comment instead.

The field settings form behavior for fields that have stored data already is a bit weird, as it outputs a message that field settings can no longer be changed, but actually does not prevent the form from submitting. That's a different problem and should be tackled in a separate issue.

Status: Needs review » Needs work

The last submitted patch, drupal.field-instance-settings.39.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
12.27 KB

should take care of a few fails.

Status: Needs review » Needs work

The last submitted patch, field_ui_instance_settings_form-552604-41.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.83 KB

Fixed some more tests.

sun’s picture

Screenshots (with patch applied).

sun’s picture

Sorry, screenshots in #44 actually map to attached patch.

sun’s picture

yched’s picture

Status: Needs review » Reviewed & tested by the community

Gave it a final test and code review, this is good to go.

Summary :
In current HEAD, Field-level settings are present in both
- The 'Field settings' page, that displays a warning that some settings are not changeable any more when the field has data. The 'Number of values' select dropdown is missing there.
- In a separate fieldset at the bottom of the 'Instance settings' page. The 'Number of values' property is displayed, but not the warning about unchangeable settings. Attempts to change an 'unchangeable setting' (in case the field type module fails to disable the corresponding form element) are not detected there and can produce unpredictable effects.

The patch :
- removes the 'Field settings' fieldset at the bottom of 'Instance settings' page. Field settings are on 'Field settings', instance settings are on 'Instance settings'.
- adds back the missing 'Number of values' select in the 'Field settings' page. This also prevents the WTF effect when creating a field with a type that has no settings, and stumbling on a 'Field settings' step with nothing to set.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

So I spent about an hour with this patch tonight with backup from sun on IRC.

Things I like about this patch:
- There are no longer two places to configure fields. That was dumb.
- One of said places is no longer NOT validated. That was really dumb.

Things I don't like about this patch:
- It doesn't solve the actual issue. Adding a new field still leads to a confusing "Field settings" form all by itself. And now it's worse because we have another confusing "Field instance" form all by itself and we force site builders to understand Field API internals enough to know the difference.
- It moves around the cardinality setting which breaks UI freeze, several books, and various documentation, not to mention 1000s of existing D7 sites who expect to find that setting on the second form and not the first, and have since CCK D5. (or maybe even 4.7)
- Because it moves this setting, which can always be changed, to a form that displays a "Settings here cannot be changed" error message after data is added to the field, it introduces a UX bug where instead of this message usually lying to people, now it will always lie to them.

So on the whole, it's about a wash. :\

This is what I would really, really like us to do instead:

1. Actually remove the field settings form. It's not needed. The field gets created in the database with default schema values without me ever having to save that form.
2. Add validation to the field settings form that's living at the bottom of the field instance form, along with all the other settings. It's definitely a nasty bug that that isn't happening.
3. Instead of displaying that error string as a non-targeted dsm('error') that lies half the time, append it to the field description instead in the case where it's a disabled field.

When I suggested doing this in IRC, sun said that the only way we could do this is to also cram in the widget settings on the same form, which would be another big change and likely one better suited for D8. Though this might be nice, I actually disagree that the widget settings form needs to move. Why? Because when you add a field, the "field stuff" and the "widget stuff" are already two separate concepts in the user's mind:

Settings are broken down between Fields and Widgets

So two separate links going to two separate settings forms kind of makes sense. At least in the "what we can reasonably do for D7" world. (In D8 we should probably re-do this entire thing to be more WYSIWYG or... something else better.)

What they are not exposed to here though is the difference between a "field" and a "field instance". And this level of detail is totally irrelevant for them to be exposed to until much later in their learning process when they discover you can re-use fields among bundles and they can come up with an actual use case for doing that. I think of all the Drupal sites I've ever built, I've used this feature maybe 5 times. So forcing this knowledge on hapless, non-Field API guru site builders who are still trying to wrap their heads around the fact that content types are "entities" and you can add "fields" to "entities" and here's what "fields" are is way, way too much. Hence the "major" priority of this issue, and hence why I don't like sun's fix. It only makes this problem worse.

Because, if you try and write hands-on instructions for a complete newbie on how to add a field to a node type in HEAD, you end up writing something like this:

1. Go to admin/structure/types and click "manage fields" next to "Basic page".
2. Under "Add new field," fill in the following values, and click Save:
- Label: Number field
- Name: number
- Field type: Decimal
- Widget: Text
3. At the first field settings form, choose a precision value of 16 and click Save field settings
4. At the second field settings form, enter the allowed values of "1.030" and "3.203" and click Save settings

It's really, really sad.

If there's only two forms for them to care about here -- one for "everything to do with fields" and one for "everything to do with widgets" -- it radically simplifies things and also matches more closely to what users are used to from CCK.

webchick’s picture

Ok, after a detailed IRC session, here's the plan sun and yched and I agreed to.

The changes #48 asks for is too much for D7 as the UI change would be pretty drastic and yched has concerns with it. Similarly the patch in #45, though technically correct, also alters the UI too much for D7 -- the "all in one" field settings form gets removed, which many users depend on since it's been there since early versions of CCK.

So we've decided to (mostly) respect UI freeze, and scale back to only fix the actual bugs in this issue, which means:

1. Add the cardinality selector to the solo field settings form. This is a field-level setting; changing it once will change it everywhere, so it makes sense to show it with the rest of the field-level settings.

2. Fix the lack of validation on the "all in one" field settings form. If there are fields that need to be "locked" once the field has data in it, they should be #disabled.

3. Fix the just plain wrong message that says none of the field settings values are changeable once the field has data in it. Cardinality, for example, will always be changeable with field_sql_storage. Replace with a more loosey-goosey worded one like "Because this field has data in it, some of its settings may be locked and unchangeable." This message needs to appear both at the top of the field settings form and on top of the fieldset in field instance form.

KarenS’s picture

Yeah. The original idea was to have a field settings form that had only the unchangeable stuff on it and show only the changeable stuff on the regular edit form, but when that was created there was no way to differentiate between them, so *temporarily* they were shown in both places. And that never got fixed. And the message about not changing settings would have made sense if it was done that way -- you can't change the stuff on the unchangeable form, you can change the stuff on the regular edit form. And since the cardinality is changeable, it went on the regular edit form.

I also thought it would be nice to add information about the widget to the manage fields screen, which we don't have in D6, with a link to change the widget, and remove that part from the global edit form.

So that's more or less the 'why it was done this way' originally.

KarenS’s picture

In other words, the idea was to not say anything about 'field settings' or 'instance settings', just differentiate based on settings that can or cannot later be changed, with all the settings that are changeable and non-problematic on the big edit form and the others tucked away into the field and widget forms.

webchick’s picture

Hm. Well #50 makes a heck of a lot of sense. So basically just remove the non-changeable fields from the all-in-one form and make the distinction between the two forms "set 'em up once and never change 'em" vs. "configure 'em any time" rather than "field settings" vs. "field instance settings"? Cool!

I think the problem though from talking to yched is that we don't have an easy, standard way of knowing what fields are changeable and what are not. It'd be the responsibility of the field module author to get this right, and they probably won't. Hence the confusingly worded error message. And some are not quite black and white. For example, list items' allowed values. It's fine to add new ones after the field has data, but not ok to change the keys of a list item that has data associated with it. (Or so is my understanding.)

Still though, Karen's approach is even easier if we can figure out a way to do it.

mlncn’s picture

Subscribing. I have read the entire issue, but i'm not clear on if this will / can result in removing the completely pointless first Field settings page when there are no field settings.

Namely, this screen:

Field settings

These settings apply to the Example field everywhere it is used. These settings impact the way that data is stored in the database and cannot be changed once data has been created.
Example has no field settings.

webchick’s picture

Version: 7.x-dev » 8.x-dev

It does not remove it, no. :( I tried to summarize in #48.

And this issue is fully D8 now, although a D7 appropriate patch (separate issue) would be fixing the validation of the second copy of the field settings form on the instance settings page.

Shai’s picture

sub

David_Rothstein’s picture

For anyone who's interested, I implemented @webchick's preferred idea in #48 (getting rid of the separate "field settings" form entirely) as part of the Backports module in Drupal 7.

See: #1277270: Remove the separate field settings page

The code as written probably isn't that useful for an eventual Drupal 8 patch, but I thought I'd mention it anyway. (It also may be useful if anyone wants to click around and see what the UI would look like if we wind up going in that direction here.)

David_Rothstein’s picture

In my own testing of it, it mostly seemed to work well, but I saw some issues when creating List fields.

There, the current workflow of having two forms actually makes a lot of sense. On the first form, you enter the allowed values (which is basically the most important thing you want to focus on when adding a List field), and then when you get to the second form they are already saved, which means that you are able to select one of them in the "default value" dropdown if you want to.

When everything is on one page, though, the allowed values are hidden down at the bottom of the form, and furthermore, since they don't exist yet, you have no options that you can choose from for the "default value". You have to enter the allowed values at the bottom of the form, save the field, then go back and re-edit if you want to set a default value. This is not a great user experience.

KarenS’s picture

There are other kinds of fields where decisions made on the first form affect what appears on the second, esp anywhere that those field settings affect the choices made later (or when those choices affect the way the default value is constructed). Also we have some settings that can later be changed and some that cannot, and it makes sense to have them in separate places, and for the unchangeable settings to be somewhat hidden by not being on the main edit form. And the other issue is that the main edit form is already really too long for good UX, I don't think we want to make it longer by adding back all the things that got moved off of it.

I think the fundamental problems are 1) some fields have no settings and we should really not make a stop on the settings form when a new field is created if there is nothing to do/choose, 2) some values are showing up in two places but they should show up in only one, and 3) The first form should really be a somewhat hidden form that is limited to values that cannot or should not be altered after there is data and the second form, which is easly discovered, should include values that can safely be changed even if there is already data.

webchick’s picture

Could these types of dependency problems not be mitigated with the use of AJAX or the #states system, though?

David_Rothstein’s picture

Yes, AJAX could fix that probably. But there's the question of order too (what fields should appear next to each other to make the relationships clear).

It's worth pointing out that any problem along these lines is actually a pre-existing bug/issue, just one that would be more noticeable after this change. (Because even in D7, for example, after a field is created you can go back and change its allowed values later, and the default value options will not update to reflect those until the next page load.)

Bojhan’s picture

Assigned: sun » Unassigned
Issue tags: -Needs usability review

I am not quite sure what to review here?

sun’s picture

Status: Needs work » Needs review
FileSize
17.13 KB

We absolutely need to get this resolved for D8.

Attached patch is a plain re-roll of #45 against HEAD, which obviously had many conflicts. I didn't test it manually yet.

Status: Needs review » Needs work

The last submitted patch, drupal8.field-instance-settings.62.patch, failed testing.

tim.plunkett’s picture

Priority: Major » Normal

I've added this to #1770720: [META] Gradual changes to Field UI and am demoting.

Gábor Hojtsy’s picture

Issue tags: +D8MI

This would also solve / help solve a usability WTF for translatable fields, since you cannot actually set the translatability of a field when setting it up initially due to certain things not appearing in the form :/ #1831608: Show or hide the "Make field translatable" checkbox on the add field form depending on translatability was marked postponed on this one.

Lukas von Blarer’s picture

I rerolled the patch and fixed the test issues.

Here is how the UI looks like now:

1. Choose field name and type (nothing has changed)

Screen Shot 2012-11-06 at 4.35.53 PM.png

2. Make global field settings (All global settings are available now)

Screen Shot 2012-11-06 at 4.36.12 PM.png

3. Make field instance settings

Screen Shot 2012-11-06 at 4.36.32 PM.png
Screen Shot 2012-11-06 at 4.37.08 PM.png

4. Editing global settings after it contains data displays a message that doesn't make sense anymore since global values can be changed.

Screen Shot 2012-11-06 at 4.37.39 PM.png

Local tasks wording

I find the wording of the tabs misleading. A first suggestion which does make it clear to me, but maybe not understandable for other people:
"Edit" -> "Instance settings"
"Field settings" -> "Global settings"

Lukas von Blarer’s picture

Status: Needs work » Needs review
Issue tags: +D8MI

Status: Needs review » Needs work

The last submitted patch, 552604-field-ui-65.patch, failed testing.

Bojhan’s picture

@Lukas Awesome that you are working on this! It looks like a good solution, I would however like to split off this multilingual fix from what this issue is about. Because your solution doesn't really solve what this issue is about, could you perhaps open a new issue for your solution specifically?

Gábor Hojtsy’s picture

KarenS’s picture

There is a difference between things like the 'number of values' setting (which can be changed at any time, and things that affect the database storage (like the type and length of a field). The number of values can be changed at any time and it is wrong to indicate that it cannot. Other things cannot be changed later. In some cases it is physically possible to change them but a bad idea (you might mess up your data).

The most logical way to construct this is to make the first form things that should be set once and never changed and the second one things that can be changed any time. The problem is that the first form is used to create the field, which means you need to have all the field settings, including those that can be changed later. So the first form has to ask what it needs to know to create the field, not just for things that cannot be later changed.

The second form can be more flexible. There we can display only things that can be changed (which includes the number of values). The problem here is that we don't have a good way to know which field values can be changed because we never implemented an API that would tell us that. I'm not sure we can fix this form without fixing that underlying problem.

sun’s picture

FileSize
5.64 KB

Hm. The interdiff in #66 is actually not complete. I've manually constructed an interdiff against #62.

The big difference seems to be that #66 introduces an entirely new form submit handler, whereas earlier patches did not have to do that. Was there any particular reason for that?

sun’s picture

Status: Needs work » Needs review
FileSize
6.64 KB
18.03 KB

That additional submit handler didn't appear to be necessary.

Attached is a straight re-roll of #62 against HEAD.

Also attaching an interdiff against #66.

@Lukas von Blarer:
I somewhat like your suggestion of changing the tab titles to "Instance settings" and "Global settings". However, I think that should happen in a separate issue.

@KarenS:
Yes, we're definitely missing an API there. However, that doesn't really change the fact that the global field settings really shouldn't be exposed on the instance edit form, since touching those performs changes to something completely else than the context of the form suggests (editing instance settings).

Status: Needs review » Needs work

The last submitted patch, drupal8.field-instance-settings.73.patch, failed testing.

Berdir’s picture

@sun: We've talked quite a bit about this in IRC and our sprint, the problem is that there are multiple possible directions and according to Bohjan, adding more things to the initial form is the oppositive of what they want. ( Which is also the opposite of #1831608: Show or hide the "Make field translatable" checkbox on the add field form depending on translatability, so consider me confused).

I totally agree with you that this is technically the correct solution, certainly compared to the mix that we have right now, but it seems what the UX team wants is to merge those two settings pages into one.

+++ a/core/modules/translation_entity/translation_entity.moduleundefined
@@ -558,7 +558,7 @@
+function translation_entity_form_field_ui_field_edit_form_alter(array &$form, array &$form_state, $form_id) {
-function translation_entity_form_field_ui_field_settings_form_alter(array &$form, array &$form_state, $form_id) {
   $field = $form['#field'];

This was done to implement the mentioned issue as a proof of concept. Which also required the $form['#field'] change from above.

That said, this should probably be done in that issue, as a follow-up, as it will also require documentation updates and so on, which would make this patch unecessarly bigger. *If* this issue is actually implement like this. If not, then that other issue might become irrelevant anyway.

@KarenS: Apart from what sun said, the field types are also very inconsistent in regards to what they allow/prevent. Changing the cardinality to a lower value than you have entities with number of items is allowed, e.g. removing allowed values that are used is not (which in most real world examples that I've encountered - during development, is a huge pain, as it's not even possible through the API, e.g. with features).

Lukas von Blarer’s picture

In my opinion there are three types of settings for a field:

Global settings which can't be changed after field creation (data structure)

  • Field name
  • Field type
  • Field specific setting E.g.: Text field length

Global stetting that can be changed after field creation but and can be potentially destructive

Number of values
Translation
Field specific setting E.g.: File field upload destination

Field instance settings

Label
Help text
Required
Field widget (I don't see the requirement of that being on a separate page)
Field specific setting E.g.: File field upload destination

I think we should group the settings similar to this and present them on one page visually clrearly separated with fieldsets or similar.

yched’s picture

I fully agree that the current Field UI organisation is not optimal.

However, do we really want to reshuffle this now (which IMO can easily take another couple 10s of comments, between those that want to split and those that want to merge), and start over again when core has an official Popups API that we're officially allowed to use ?
Or start to come up with a solution that works well with popups ?

sun’s picture

re #75:

adding more things to the initial form is the oppositive of what they want.

Mmm, I'm confused — this patch here does not "add more things" to the initial form. It only removes the form elements from the second form, which are on the initial form already.

In short, the difference is this:

Before:
1) Field settings form
2) Widget settings form
3) Instance AND Field settings form

After:
1) Field settings form
2) Widget settings form
3) Instance settings form

Whereas 3) is especially confusing when adding a new instance by re-using an existing field — the instance settings form suggests that you'd be able to edit the global field settings, too (I'm allowed and supposed to change the controls that I'm presented with, right?) and gives no indication or whatsoever that touching those controls will have a site-wide impact on the field wherever it is used.

The global field settings controls should not be exposed on the instance settings form to begin with.

That is the sole and only purpose of this issue since the beginning. This was originally discovered very late in the D7 cycle, and it was deemed to be too late to change it for D7 (#49). Very soon, we are in the identical situation for D8.

For the sake of sanity and doing pragmatic progress, let's please, please, please not make that same mistake again. :)

We can still completely revise this flow and do whatever we think would be nice, but let's please do that in a dedicated follow-up issue.

Gábor Hojtsy’s picture

I think if we can do this simple fix quickly, then it would be great to get it in ASAP and then work on even more rework later. Leaving it in the D7 stage would be pretty sad. If we are hooked up here discussing how to do it for a non-trivial time, than it is not worth it to do in multiple layers (IF somehow the better improvement happens sometime).

yched’s picture

Right, sorry, got confused by some comments above.
#78 is a plan I'm perfectly fine with.

sun’s picture

Status: Needs work » Needs review
FileSize
20.52 KB

Fixed all test failures.

Berdir’s picture

@sun

- The form that I meant is the one you see when adding new fields (see issue title :)). Right now, you often have very few or no settings at all there, this patch means that all field settings show up there. Bohjan wants to get rid of that step completely.

- Make sure you test your changes with translation_entity.module enabled. Without changing it, it means that the setting which it adds shows up outside of the fieldset on the instance settings form.

sun’s picture

1) Yeah, understood. :) There's room for rethinking this user interface entirely. But let's first make sure that we don't continue the current weirdness. :)

2) Thanks for the pointer — manually tested that and adjusted it.

Berdir’s picture

1. In case I wasn't clear. I agree 100% with you. And I tried to explain it to Bohjan but wasn't very successful :). He's the person you need to convince that it makes sense to do this as a first step and *then* try to improve it further.

sun’s picture

@Bojhan: I hope we can agree on a pragmatic intermediary commit to facilitate progress. :) I'm waiting for this clean-up to happen since August 2010 (#25) and I wouldn't like to see us run into the same pitfall as with D7. ;) It almost looks like it would be best to retain this issue for further mockups and discussions afterwards, since there's plethora of background, technical, and usability information in the above comments already.

Bojhan’s picture

@sun I dont get the solution, and or how it moves us closer to solving this issue. All it seems to do is change some labels, and trow some fields around. It probably doesn't make it worse, so from that pov I am fine with committing it.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

I like the change, it cleanly separates the configuration on the page it needs to be - granted, probably more on technical reasons (fields vs instances vs widget), but then again, it makes sense. We can always clean up more in follow ups, so let's get this in!

sun’s picture

YesCT’s picture

Updating the issue summary might make this easier for a committer to evaluate. (It was RTBC since Nov 9 and has not gotten in) Anyone up for re-doing the issue summary?

sun’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Shucks. This one no longer applies due to the details patch. REALLY happy to see it RTBC though! Can we get a quick re-roll?

swentel’s picture

Status: Needs work » Needs review
FileSize
23.94 KB

Here's the re-roll, should be fine I think.

Status: Needs review » Needs work
Issue tags: -Usability, -Needs issue summary update, -Fields in Core, -String freeze, -API change, -D8MI

The last submitted patch, 552604-92.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Needs issue summary update, +Fields in Core, +String freeze, +API change, +D8MI

#92: 552604-92.patch queued for re-testing.

swentel’s picture

Test passes on my local machine and the failing one was in views which really doesn't have anything todo with this patch.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

And moving back to RTBC

YesCT’s picture

manual testing, screenshots, trying to understand the scope of the (proposed) fix

00a. screen shots with 8.x edit field (shows all global settings in a collapsed/expanded fieldset at bottom)
confusing-s00a-nopatch-editfield-New Thing | confusing-552604-92 2012-11-27 10-35-42.png

00b. and 8.x field settings (only some of the global settings, no number of fields for example, has warning cannot change these)
confusing-s00b-fieldsettings-New Thing | confusing-552604-92 2012-11-27 10-37-03.png

with patch

01. creating a field on manage fields
confusing-s01-add-field-manage-fields-2012-11-27_1004.png
02. after save global field settings
confusing-s02-after-save-global-field-settings-2012-11-27_1006.png
03. non global settings edit field page
confusing-s03-non-global-settings-edit-New Thing settings for Article | confusing-552604-92 2012-11-27 10-09-10.png
04. make a node with content in the field
confusing-s04-onenode-hascontent-2012-11-27_1010.png
05. mangage fields
confusing-s05-managefields-2012-11-27_1013.png
06. edit field (shows just the settings that are not global, no global collapse/expand)
confusing-s06-editfieldwithcontentexisting-New Thing settings for Article | confusing-552604-92 2012-11-27 10-14-56.png
07. field settings (show all global settings, with wanrning cannot change. but can change them)
confusing-s07-fieldsettingswithcontentexisting-2012-11-27_1016.png

seems like plan is to fix this like that here.

then make follow-ups for remaining things to change, like:
fixing the red [edit: read->red] warning,
relabeling the tab to global settings..
were there any other follow-ups that need to be opened?

swentel’s picture

Assigned: Unassigned » swentel
Status: Reviewed & tested by the community » Needs work
swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
30.16 KB

Re-rolled.

YesCT’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

Looks like webchick's already given this a good, look, and it's had to be re-rolled a few times, so committed this while it still applies. Thanks!

YesCT’s picture

this commit (git checkout 08df7b0db2369bb) introduced #1872786: Can't change widget types

webchick’s picture

That is actually a bit more nefarious than mere notices; it prevents you from changing the widget type at all. Escalated to a critical.

David_Rothstein’s picture

Status: Fixed » Active

According to #85 and others, this was only an intermediary commit and the issue should remain open. (Or at least, a new issue should be filed as a followup.)

swentel’s picture

Assigned: swentel » Unassigned

@YesCT - I guess we can close this one now then with the new ones ?

David_Rothstein’s picture

Well, the main new issue that's needed (or the reason to keep this open) is for the fundamental usability issue that you still get redirected to a confusing Field Settings form (and confusing multi-step process involving fields vs. instances) in order to simply create a field.

So for example, #48 or other ideas mentioned above...

Gábor Hojtsy’s picture

Issue tags: -API change

Not an API change anymore. Cleaning out the API changes proposed against Drupal 8.

tim.plunkett’s picture

Issue summary: View changes
amateescu’s picture

Assigned: Unassigned » amateescu

After #1963340: Change field UI so that adding a field is a separate task got committed, I started working on this one but I hit a roadblock with entity forms not being flexible enough for the use case of field_storage_config forms: #2399219: Allow entity form handlers to determine the entity object they need to work with. I'll resume the work and post a patch here when that issue is fixed.

andypost’s picture

@amateescu please update summary with approach you trying to implement, the blocker is in

xjm’s picture

Status: Active » Postponed

If this is blocked on #2446869: Convert the "Field storage edit" form to an actual entity form should it be postponed? Also holy cow at this issue; six years old. :)

Coming here from #1847596: Remove Taxonomy term reference field in favor of Entity reference, where it was suggested that this issue could resolve this problem, described in #261 of that issue:

That doesn't seem quite in the main scope here though, so maybe it should be a followup issue postponed on this one?

amateescu’s picture

Sometimes I just get too busy working on stuff so I forget to document all the issue dependency chains I'm going through :/ But you're right, that specific problem should be solved in a followup to this issue.

xjm’s picture

jibran’s picture

amateescu’s picture

Status: Active » Needs review
Issue tags: +D8 Accelerate London
FileSize
29.2 KB

I worked a bit on this during the D8 accelerate sprint in London and since we're still in the beta phase and UIs are not frozen, I think it's still a good time to get it done.

Here's a patch that works and is ready for architectural reviews, I'm going to wait a bit for feedback before updating all the tests :)

amateescu’s picture

Discussed the addition of labels to field storage configs with @alexpott and he feels that it's a a bit out of scope for this issue. I only added it because it makes it easier for the form embedding part, but we can also work around it somehow if needed.

amateescu’s picture

FileSize
21.47 KB
12.75 KB

Also discussed with @yched and he agreed that we shouldn't add labels to field storage config entities here, so I removed that part from the patch.

Bojhan’s picture

Can you explain what impact this has on UX? I dont want to just descope a important UX aspect, because we think it doesn't belong.

amateescu’s picture

FileSize
23.95 KB
14.86 KB

@Bojhan, the discussion about the label element doesn't have any impact on UX, it's still shown in the same place with the latest patch :)

Discussed this patch at length with @yched and he suggested the following improvements.

Note that there is still a bug in there which doesn't allow you to upload a default image for an image field, but I wanted to post the current work just in case someone wants to take it for a spin and do a UI/UX review.

Gábor Hojtsy’s picture

Given that many improvements in the field UI scene seem to depend on this, rerolled. Also looking at test results for once.

Status: Needs review » Needs work

The last submitted patch, 122: 552604-122.patch, failed testing.

Gábor Hojtsy’s picture

Adding some screenshots to the issue summary. Are we sure we want it to work this way still?

Bojhan’s picture

Can anyone explain to me why we can't have 2. on the same screen and just reload settings with AJAX when someone changes something above (and lock after creation)?

amateescu’s picture

@Gábor Hojtsy, thanks a lot for the screenshots! They help a lot with reviewing the UI impact of this change :)

@Bojhan, if you mean step 2 from the 'after' screenshots, we can certainly do that, but it would be better suited for a followup so we can keep the patch here in a reviewable size.

Gábor Hojtsy’s picture

@Bojhan: agreed with @amateescu, a first step here is to merge the two forms with the Ajax experience. We can merge the third one later if we believe that makes sense. Those two are currently on two separate tabs too later on when you edit the field, so that needs more consideration as well. The two forms currently merged are not appearing separately in the later flow so they don't pose such questions.

Gábor Hojtsy’s picture

@amateescu do you have time/interest to help bring this up to passing again? :)

amateescu’s picture

@Gábor Hojtsy, of course, that's why I kept it assigned to me. Just like you asked in #124, I'm waiting for a confirmation that we still want to do this given all the recent discussions around "the great Field UI overhaul" :)

Bojhan’s picture

Ok, lets do this first step then.

Gábor Hojtsy’s picture

@Bojhan: Thanks for the go-ahead :) @amateescu: can you take it forward? :)

amateescu’s picture

Version: 8.0.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
31.82 KB
8.75 KB

Yup. Let's see how far these fixes take us.

Status: Needs review » Needs work

The last submitted patch, 132: 552604-132.patch, failed testing.

jibran’s picture

+++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
@@ -160,38 +166,80 @@ public function buildForm(array $form, FormStateInterface $form_state, $entity_t
+      '#attributes' => [
+        'id' => 'new-storage-wrapper',
+      ],

No need to use #attribute we can set #id directly for container.

Berdir’s picture

No, we can't. That's an #ajax wrapper and must remain the same, #id has special handling that generates new unique ID's on ajax updates.

jibran’s picture

Yes, we can. :) I know this is an ajax form and it has to remain same all the time that is why in D7 theme_container or D8 template_preprocess_container doesn't pass it to drupal_html_id or Html::getUniqueId. This is a common misconception which we always miss, container type element supports #id for exactly same reason so that it can't be changed.

amateescu’s picture

Status: Needs work » Needs review
FileSize
39.02 KB
8.43 KB

Fixed the rest of the tests and #134.

Status: Needs review » Needs work

The last submitted patch, 137: 552604-137.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
38.97 KB
839 bytes

A debugging change crept up in there :/

jibran’s picture

I tested it on all the available field_ui add-form in standard profile for user ER, content ER, comment field and text field it works like a charm. This is RTBC imo but let the usability team review this.

+++ b/core/modules/field_ui/src/Form/FieldStorageAddForm.php
@@ -427,6 +464,62 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+    if (strpos($field_type, 'field_ui:') !== FALSE) {
...
+      $field_type_class = $this->fieldTypePluginManager->getDefinition($field_type)['class'];
+      $field_options = $field_type_class::getPreconfiguredOptions()[$option_key];

We should add comments explaining these.

amateescu’s picture

Assigned: amateescu » Unassigned
Issue tags: +DCTransylvania

Note that there's still one bug mentioned at the end of #121 that we need to fix. I'll try to track it down in a couple of days but anyone is welcome to give it a try before then :)

  • catch committed 08df7b0 on 8.3.x
    Issue #552604 by sun, lambic, swentel, yched, Lukas von Blarer: Adding...

  • catch committed 08df7b0 on 8.3.x
    Issue #552604 by sun, lambic, swentel, yched, Lukas von Blarer: Adding...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drugan’s picture

The issue of a field add/manage confusing interface is still in place as I think so I suggest to move field settings to their relative widget/formatter settings page. Some attempt is done to resolve this issue for the number field's widget. Motivations are explained here.

  • catch committed 08df7b0 on 8.4.x
    Issue #552604 by sun, lambic, swentel, yched, Lukas von Blarer: Adding...

  • catch committed 08df7b0 on 8.4.x
    Issue #552604 by sun, lambic, swentel, yched, Lukas von Blarer: Adding...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Note that there is still a bug in there which doesn't allow you to upload a default image for an image field, but I wanted to post the current work just in case someone wants to take it for a spin and do a UI/UX review.

NW for #121.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Pancho’s picture

Indeed, commits #142 to 147 seem to be unrelated, and this issue remains relevant.

Therefore, here's a rather straight reroll of #139. Almost no hunk applied anymore, so I had to port most changes manually. While I tried my best in not changing more than absolutely necessary, I didn't manage to create a usable interdiff, so please give the code another thorough review. Also, for now, I skipped the tests, so we might have a few test fails.

From my manual testing, I don't think we're 100% there, but already this seems to be a major improvement over our current UI.

Pancho’s picture

With JS being disabled, this (both #139 and #153) doesn't work at all.

Suggest we add a non-JS "update" button (like in "Add view" etc.) that rebuilds the form in the case of adding a new field. And suggest we do that before reinstating/fixing test coverage, because otherwise we need JavascriptTestBase/FunctionalJS for every single test which seems superfluous for many aspects.

Also, label and machine_name fields need to be marked required.

Pancho’s picture

As suggested above, I made the non-JS version work using a (non-JS-only) update button. Label and machine_name are now properly marked required, too (Don't really get why JS styling was necessary at all).

Also merged in some code from #3037625: Duplicate label field when adding field w/o JS by @mcannon (don't want to postpone on that one, please credit if committed). We actually don't need a Label field at all at this point when adding an existing field. While the label can still be changed right in the next step, both UI and code may be significantly simplified.

Tests still missing plus quite a number of them will be broken. I will take care of that.
In the meantime, please test (both with and w/o JS) and review whether the approach is what we want.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

andypost’s picture

Status: Needs work » Fixed

This issue should be closed because already commited
If feedback from #121 still needed, please file follow-up

Status: Fixed » Closed (fixed)

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