The fact that complex IEFs are rendered in a table cell means the additional fieldset wrapper seems completely superfluous and often visually annoying. It can be easily overridden using the CSS rule display: contents, however in most usecases it will be unwished for, so it should go.

Even for simple IEFs a fieldset isn't necessarily wanted. To provide a seamless integration of the IEF, sitebuilders may not want any visual separation. Or they might want to put the IEF in a vertical tab instead. The only difference is that in the basic usecase a fieldset helps understanding how the IEF works.

Initially I didn't want to add yet another setting, being rather in favor of referring sitebuilders to the Field group module that allows all kinds of fancy fieldsets, vertical tabs etc. However, it would be a bit irritating to force it for simple IEFs (though sitebuilder might want to get rid of it), yet do away with it for complex IEFs. Also, we're in RC phase, so we don't want to remove any functionality.

Therefore I am adding another setting, though using #states to expose only relevant settings. Default for simple IEFs would be "on", default for complex ones "off."

CommentFileSizeAuthor
#10 ief_allow_seamless_integration-3030273-10.patch5.91 KBershov.andrey
#7 ief_allow_seamless_integration-3030273-7.patch5.91 KBnevergone
#2 in-fieldgroup-seamless.png33.2 KBpancho
#2 in-fieldgroup-before.png35.24 KBpancho
#2 simple-seamless.png15.08 KBpancho
#2 simple-before.png16.08 KBpancho
#2 ief_allow_seamless_integration-3030273-2.patch5.7 KBpancho

Issue fork inline_entity_form-3030273

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Pancho created an issue. See original summary.

pancho’s picture

pancho
Location UTC+2 🇪🇺 EU
commented
Title: Don't display complex IEFs in a fieldset by default » Allow hiding fieldset and title
Category: Task » Feature request
Status: Active » Needs review
StatusFileSize
new ief_allow_seamless_integration-3030273-2.patch5.7 KB
new simple-before.png16.08 KB
new simple-seamless.png15.08 KB
new in-fieldgroup-before.png35.24 KB
new in-fieldgroup-seamless.png33.2 KB

Here's a patch (no tests yet) and some screenshots:

Simple IEF Complex IEF,
with some other fields in a fieldgroup
Before:
before 		Before:
before
Seamless (fieldgroup and title hidden):
seamless 		Seamless (fieldgroup and title hidden):
seamless
nevergone’s picture

nevergone
Primary language Hungarian
commented

I'm tested and works well! Thanks!

neeravbm’s picture

neeravbm commented

I just installed the patch. It works as expected but I am not sure whether this configuration is really needed in the module.

extect’s picture

extect commented
Status: Needs review » Reviewed & tested by the community

Patch is working for me as well. Any chance to get this committed?

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Needs work

Overall I'm a bit wary about adding yet more configuration options... at least I don't think these will affect form functionality.

  1. +++ b/config/schema/inline_entity_form.schema.yml
@@ -60,6 +66,12 @@ field.widget.settings.inline_entity_form_complex:
+    hide_fieldset:
+      type: boolean
+      label: "Hide fieldset"
+    hide_title:
+      type: boolean
+      label: "Hide title"

    Negative labels are generally a bad idea, because the user has to think about a double-negative when the checkbox is unchecked: 'don't hide == show'.

    However here we have the problem that the default behaviour is to show, so to make the checkbox a positive, we'd need to enable the property in existing fields with an update hook. 

  2. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormBase.php
@@ -237,10 +239,30 @@ abstract class InlineEntityFormBase extends WidgetBase implements ContainerFacto
+      '#title' => $this->t('Hide element title'),

    What does it mean if you hide the fieldset but show the title? Where does the title go? 

  3. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormComplex.php
@@ -209,7 +209,7 @@ class InlineEntityFormComplex extends InlineEntityFormBase implements ContainerF
+      '#type' => 'fieldset',

    I don't really like this pattern of setting the #type then changing it later.

    Clearer to either set it in one go, or don't set it here and set it for all cases further down. 

  4. +++ b/src/Plugin/Field/FieldWidget/InlineEntityFormSimple.php
@@ -36,13 +36,20 @@ class InlineEntityFormSimple extends InlineEntityFormBase {
-      '#type' => $this->getSetting('collapsible') ? 'details' : 'fieldset',

    Same here.

nevergone’s picture

nevergone
Primary language Hungarian
commented
StatusFileSize
new ief_allow_seamless_integration-3030273-7.patch5.91 KB

patch re-rolled (rc9)

jasmina4w’s picture

jasmina4w commented

In modules/contrib/inline_entity_form/inline_entity_form.api.php, you have hook_inline_entity_form_entity_form_alter. With this you could add your #attached.

socialnicheguru’s picture

socialnicheguru commented

There is always the option to come up with a form view mode for the entity in question that will hide the fields that you want and be used with IEF.

ershov.andrey’s picture

ershov.andrey commented
Status: Needs work » Needs review
StatusFileSize
new ief_allow_seamless_integration-3030273-10.patch5.91 KB

Updated patch for 8.x-1.0-rc14

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Version: 8.x-1.x-dev » 2.0.x-dev
Status: Needs review » Fixed

tnx

Status: Fixed » Closed (fixed)

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

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Version: 2.0.x-dev » 3.x-dev
Status: Closed (fixed) » Needs review

Bulk reopen.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Sounds reasonable. Needs tests though.

facine made their first commit to this issue’s fork.

facine opened merge request !147

facine’s picture

facine commented

I've created a MR porting the these changes, but we still need tests.

tvalimaa’s picture

tvalimaa commented

#17 MR changes are looking to work. Thank you.