Spin-off from #1770720: [META] Gradual changes to Field UI. This is just the first step in a refactoring process field ui needs.

Regions in Field UI

Now we have "main", "visible", "hidden" as default regions and separate row handlers to work with, etc ... . To alter the fields UI screens, it's very difficult and inefficient altering to obtain things that should be transparent or done behind the scene.
The solution is to use content/hidden so the concept of layouts can more easily be introduced later.

This patch includes:

  • make regions consistent across the fields and display overview screens. Important, since there is only 1 region, it won't be shown at display screen.
  • make row handling in PHP consistent for all screens. I started with "content" and "hidden" to not change too much at the same time.
  • getRegion in javascript always knows the region and not hardcoded on the hidden/main region anymore.
  • Refactor the code so the overview screens are based on the same base class. (One change is for instance that a method getRegions will define the regions and how the behavior is by default).
  • added the region column so it works with regions by default. Nesting behavior is now consistent on both screens as well.

The only visually thing is that you need to change regions to change regions.
Before you needed to set the format type to something or "hidden", which triggered the region update.

Files: 
CommentFileSizeAuthor
#11 d8-field-ui-1786198-11.patch96.36 KBStalski
#3 d8-field-ui-1786198-3.patch96.6 KBStalski
FAILED: [[SimpleTest]]: [MySQL] 41,301 pass(es), 38 fail(s), and 40 exception(s). View
d8-field-ui-1770720.patch96.29 KBStalski
FAILED: [[SimpleTest]]: [MySQL] 41,300 pass(es), 10 fail(s), and 0 exception(s). View

Comments

swentel’s picture

Status: Active » Needs review

Woot, groovy. First extremely quick review, mostly some nitpicks.

+++ b/core/modules/field_ui/field_ui.jsundefined
@@ -296,11 +298,12 @@ Drupal.fieldUIDisplayOverview.field = function (row, data) {
+    ¶

Spaces

+++ b/core/modules/field_ui/field_ui.jsundefined
@@ -308,43 +311,35 @@ Drupal.fieldUIDisplayOverview.field.prototype = {
+     // If a row is handled by field_group module, loop through the children.
+     if ($(this.row).hasClass('field-group') && $.isFunction(Drupal.fieldUIDisplayOverview.group.prototype.regionChangeFields)) {
+       Drupal.fieldUIDisplayOverview.group.prototype.regionChangeFields(region, this, refreshRows);

We should find something else here as core doesn't check on modules in contrib

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.phpundefined
@@ -0,0 +1,495 @@
\ No newline at end of file

No newline at end of file

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.phpundefined
@@ -0,0 +1,665 @@
+        'message' => t('No fields are present yet.')

Sounds weird

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.phpundefined
@@ -0,0 +1,665 @@
+        'message' => t('No fields which are not used')

Sounds a bit weird as well

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.phpundefined
@@ -0,0 +1,665 @@
\ No newline at end of file

No newline at end of file

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.phpundefined
@@ -0,0 +1,77 @@
\ No newline at end of file

No newline at end of file

Status: Needs review » Needs work

The last submitted patch, d8-field-ui-1770720.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Stalski’s picture

FileSize
96.6 KB
FAILED: [[SimpleTest]]: [MySQL] 41,301 pass(es), 38 fail(s), and 40 exception(s). View

The new patch includes:
- changes suggested by swentel, except the "fieldgroup" check. This has actually nothing to do with fieldgroup module but just makes sure there is nesting behavior. E.G. dragging "something" with field nested underneath must make sure all children are updated as well.
- The region property of fields on Display screen did not work yet, that is fixed now.
- TODO: Fix the submit of the region property on "fields UI overview screen"
- TODO: Make sure the region property for Manage display>extra fields is saved as well. This is one by a variable.

In the next couple of days, I will port fieldgroup module to see if is indeed easier to use what core provides. Also display port to D8 has started and is already usable.

swentel’s picture

Status: Needs work » Needs review

Let's see what the bot says

Status: Needs review » Needs work

The last submitted patch, d8-field-ui-1786198-3.patch, failed testing.

andypost’s picture

Issue tags: -JavaScript

Is there any screenshots?
EDIT probably related #1787634: [META] Decouple layouts from themes

nod_’s picture

Issue tags: +JavaScript

tag

yched’s picture

Issue tags: +JavaScript

I could only give this a cursory look, sorry :-(

- for now "Manage display" doesn't seem to work - "undefined index 'region' in FieldUIDisplayOverview.php line 151"
fixed this locally by turning the code to :
'#default_value' => isset($display['region']) ? $display['region'] : 'content',
dunno if that's the correct fix (the code in the patch works after you saved the page)

- +1 on using classes to group common functionnality on those forms.
Detail : now that Form API accepts object methods as "form callbacks" (not sure about validate and submit), we could possibly use that directly and get rid of the functional callback. See how entity_get_form() does it.

public function form($form, &$form_state) {
+    $form += array(
+      '#entity_type' => $this->entity_type,
+      '#bundle' => $this->bundle,
+      '#view_mode' => $this->view_mode,
+      '#fields' => array_keys($instances),
+      '#extra' => array_keys($extra_fields),
+    );

I know that's a straight port of the current field_ui_field_overview(), but there's a push to get rid of those "put business stuff in $form" habits in core, and use $form_state instead. If you're refactoring all this, might be a good opportunity to do so.

Consideraions about the 'region' select in a followup.

yched’s picture

So, while I know its your primary motivation for this, I'm still uncomfortable with the 'regions' thing :-/.

Whether we will have a notion of 'regions' in core, and at which level those 'regions' will operate (page, inner node) is still very undecided.
For now, we don't, and UI-wise I can only reason on what we have, and what makes sense for a user on a core install.

For those following the discussion : what the patch does is add a "regiion' column next to 'field name' on "Manage Fields" and "Manage Display" screens, containing a select dropdown with 'content' and 'hidden' regions.

- I'm still fairly reluctant to put "hidden" as a region on "Manage fields". Hiding a field on forms means it gets only populated programmatically, or always receives its default value and never changes.
This eems to me like an edge use case (currently adressed by the 'hidden widget' provided by field_extrawidgets / hidden_field), for which I 'm not convinced which should put such a prominent UI, with a non-minor screen real-estate cost. Can totally be debated, though, but this needs feedback from the community.

- So the only region we currently have is the "hidden" region on Manage display. Right now in HEAD/D7, visibility is controlled by the 'formatter' select : you hide a field by selecting 'hidden' as a formatter, and dragging in and out of the "hidden" region updates the 'formatter' select.
This matches what happens internally : setting to 'hidden' means setting $instance['display'][$view_mode]['type'] to 'hidden' (which btw doesn't seem to work in the current patch ?).
It also avoids having the UI show a 'hidden' field using 'image' formatter with 'style: thumbnail' settings, which seems contradictory.

So, I'm aware that having this 'region' column handled by core makes it easier for other contrib modules reason notion of regions without having to mess with the core form (and check whether another similar module hasn't added its own handling already).
We did that in D7 with "parent" select for field_group & DS - but that was knowing that this bit of extra UI had no visual impact on the UX of vanilla core (the 'parent' select is hidden by tabledrag.js).
It's quite different here : this additional 'region' select adds complexity and visual cruft to the vanilla core UX with no functionnal gain. That's a net loss :-(.

Also, I'm *very* reluctant to add $instance['display'][$view_mode]['region']. We already agree that display settings would ideally be outside of the $instance array, but adding a 'region' entry for use cases that are not provided in core doesn't look good.

In short, I'd suggest to split this in the following tasks :
- Code-wise: push the code refactoring part (FieldUIOverviewBase & friends), without the 'region' stuff for now.
This is very much needed and makes the code cleaner, more maintainable - and more extensible if need be.
- Code-wise: work on moving display settings out of $instance, and into a $display object, that lists components to be displayed gor a given bundle in a given view mode - fields, 'extra fields', and their settings. See if those $display objects can support the notion "regions" without being too invasive.
(in this approach, a "hidden" field is simply not listed in the display)
- UI wise: discuss with UI folks and the community how the notion of "regions" can be added to the current UI without degrading the core UX.

Stalski’s picture

Thx a lot for your feedback.

The code already works locally for display and field screens. Also about the code, great you like the refactoring.
I also agree on the fact that we don't need a hidden region on the field UI form by default. So this can be worked out to be able to take just one region. That would also indicate we don't need a "visual" region column. (so in this case the region column should be hidden from display like the parent and weight row.)
So in summary: the code could easily be refactored so that there is no visual change at all on the fields overview screen!

About the region column that is added on display forms, I realize what you said. For me, it feels so wrong to have to set the format_type to hidden to actually put a field in another region. It's also that part that needs to be overridden or unset by field_group and/or display suite and/or others to be able to participate in the UI forms.

Also field ui makes field_group possible by adding the possibility to "nest" things.

Ok, then all things aside. I fully agree on most things. Actually I am struggling with myself on this.
I'll create a patch for this issue to introduce regions.
Another issue will be created to have the code refactoring shizzle.

Stalski’s picture

FileSize
96.36 KB

Just uploading the patch I had untill now to track my own changes.

zuuperman’s picture

Link to the change record (+ entity form issue) for using object methods as form callback.
http://drupal.org/node/1734540

Stalski’s picture

This issue will now cover only the consistency for regions.
The refactoring of the field_ui will be handled in #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted.

- Edit - this issue is kind of blocked untill the first one gets in.

swentel’s picture

Stalski’s picture

Already included in #1792600: Refactor field_ui so common behavior for fields and display overview screens are extracted

  • "make regions consistent across the fields and display overview screens. Important, since there is only 1 region, it won't be shown at display screen."
    => The regions are now fetched the same way for fields and display screen. A small system is integrated to make sure you want region visible-in-ui or not
  • Refactor the code so the overview screens are based on the same base class (this was the main reason for splitting the issues)

TODO in this issue

  • Make row handling in PHP consistent for all screens. Also "getRegion" in javascript shouldn't be hardcoded on hidden/main region
  • Add the region column without being displayed. Preferably we only trigger this when it should, for performance reasons on big forms. I am still pondering on the the triggers / events that cause regions to be useful
  • Make sure nesting behavior is consistent on fields and display screen
  • Make sure we can potentially refresh the fields form as well. This basically means that the invisible refresh button should be available in the same way for both screens
  • (please advice for extra things)

So the format type will still be the trigger to refresh the form.

Stalski’s picture

Status: Needs work » Postponed

I worked on this yesterday and I ran into the same problems as a while ago.
In short: If there is no possibility to have multiple regions, core does not have to execute code that works with multiple regions. (E.g. extra region column, logic for the region handlers, ... )

So my decision is to first have the ability to have multiple regions. Field UI will know about the regions through the Display object.
Therefore we need the issues:
- #1814520: Create 2 basic sample layouts: 1-col and 2-col (not committed yet, but we can work with these layouts already)
- #367498: Introduce 'display' as a way to group and reuse instance and formatter settings. in combination with #1817044: Implement Display, a type of config for use by layouts, et. all

andypost’s picture

Status: Postponed » Needs work

Seems nothing is blocking this

andypost’s picture

Issue summary: View changes

Update summary

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.