Problem/Motivation
Currently, the widget provided by this module extends Inline Entity Form's "InlineEntityFormComplex" widget. Much of the functionality in the widget is focused on restructuring what is provided by Inline Entity Form to work with the Layouts API, leading to verbose, complicated code. This makes the widget inherently hard to maintain and somewhat fragile, as it depends on the specific structure produced by IEF.
Proposed resolution
Replace the existing widget with a new, simpler one that doesn't depend on IEF.
Remaining tasks
Provide patches with WIP until sufficiently stable for dev branch.
User interface changes
None.
API changes
Once stable this will be a non-API-breaking change.
Data model changes
None.
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#40 | entity_reference_layout-remove_ief-3068270-40.patch | 128.35 KB | justin2pin |
Comments
Comment #2
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedThinking through this more, I think we should replace the existing widget rather than add a new one, since the ultimate goal is removing the IEF dependency altogether. I'm changing the issue summary to note as much.
Comment #3
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedPatch attached.
Notes:
This is a complete rewrite of the field widget class, removing the dependency on Inline Entity Form. The ERL widget no longer extends IEF; it simply extends core WidgetBase.
The widget plugin source code should be cleaner, easier to understand, easier to maintain, and slightly faster from a performance perspective. Rather than modifying (i.e. wrestling with) IEF output to get the desired results, we're just building exactly the structure we need.
By default, this version does not show the Layout Options form originally provided by this module. Rather, it expects layout options to be handled by layout plugins themselves. However, to ensure this change is non-api-breaking, there is an option in the field display configuration to "Always show layout options form".
Next Steps:
This needs to be thoroughly tested in a variety of contexts before being committed to the main branch. We plan to continue testing in a number of projects underway, but appreciate any additional help!!
Comment #4
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedComment #5
BramDriesenGot 2 warnings thrown
Comment #6
BramDriesenThis results in not being able to create sections:
Comment #7
BramDriesenWanting to edit an existing ERL field results in a fatal error:
Comment #8
BramDriesenCreating a new ERL field throws the following warnings/notices:
Comment #9
BramDriesenAnd after saving the new field, I can add layout paragraphs, but you can't add paragraphs into the layout since you the "+" Icon isn't populated.
Also the grey background seems to be missing in the CSS making the UI difficult to see (on node edit/add pages)
Comment #10
BramDriesenI updated the patch a little bit. I had to include the patch from #3045458: Configuration error when changing bundles in field config as well since otherwise the settings form would be broken. My patch fixes the fatal errors & warnings however I noticed that the "+" button isn't being populated with the add paragraph settings when you selected the Exclude setting. Only include seems to be working for now. A lot of the CSS also seems to be broken and needs some work. I fixed the background colour.
Attached interdiff & patch, leaving on needs work since it's obviously not ready yet.
Comment #11
BramDriesenFixed the fatal error when editing an existing ERL field.
Comment #12
BramDriesenAlso the nesting issue from issue #3067550: ERL doesn't work inside a paragraph is not resolved with the new widget. If you try to add a layout in a ERL field which is placed in a paragraph, you get the following error which I couldn't trace back yet to the root cause.
Comment #13
justin2pin CreditAttribution: justin2pin at Aten Design Group commented@BramDriesen Thanks for all of this! I spent some time bringing CSS styles back inline with how they were working previously. There are a few very minor changes (hopefully improvements) with the new CSS, but please let me know if you notice any issues.
Setting this to "Needs review," although I'm going to continue investigating the other issue you posted in #12 above.
-Justin
Comment #14
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedOf course I posted too soon -- noticed an issue where "Add Section" wasn't showing up for new content.
Comment #15
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedUpdated patch - hoping to address #12 and #3067550: ERL doesn't work inside a paragraph. There were a number of issues with the new widget when multiple ERL fields are present on a single edit screen. I'm continuing to track those issues here -- rather than opening a separate issue -- since they are bugs with the new widget (although they are likely problems in the current widget as well).
Comment #16
lexsoft00 CreditAttribution: lexsoft00 commentedHi,
Patch fails on latest dev version.
Comment #17
justin2pin CreditAttribution: justin2pin at Aten Design Group commented@lexsoft I can't seem to reproduce that issue. Can you try patching latest dev with #15 again and let me know what happens?
Comment #18
lexsoft00 CreditAttribution: lexsoft00 commentedIt works now, thanks.
Comment #19
bgilhome CreditAttribution: bgilhome commentedPatch working well for me, except for the minor issue that closing the modal (via close or Cancel buttons) doesn't trigger dialog beforeclose/afterclose events on the window. This caused an issue for me whereby entity_browser sets body classes on dialog open/close to prevent scrolling - the classes were added on open but not removed on ERL modal close.
Patch attached ensures that those events are triggered. Interdiff to patch in 15 also attached.
Comment #20
bgilhome CreditAttribution: bgilhome commentedAnother issue I found was using the 'negate' or 'exclude' option for the target bundles in the field settings - currently the widget doesn't respect the negate setting. Patch attached to fix this, along with interdiff.
Comment #21
BramDriesenI tested the patch a bit and it seems to work fine (it broke some things that we custom developed, but that's fine since we were depneding on some hooks coming from IEF). It seems to be more responsive as well actually. I didn't test a nested example though.
Comment #22
justin2pin CreditAttribution: justin2pin at Aten Design Group commented@bgilhome Thanks for this - unfortunately the latest patch (#20) doesn't apply to dev after pushing the fix for #3075919: Ajax error without user permission to edit plugin config. I'm working on a separate patch for this issue that addresses a few other things we've found, and will fold in your changes as well.
Looking forward to getting this all pushed back to dev, which will simplify things in the future.
Thanks again.
Comment #23
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedComment #24
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedAttached patch addresses a number of issues:
Note that interdiff is from #15, since #19 and #20 no longer apply cleanly to dev.
Review and feedback would be much appreciated... would love to get his pushed to the main dev branch!
Comment #25
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedComment #26
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedRerolled patch against latest dev.
Comment #27
AnybodyThank you for this very important improvement. We just tried this patch to find out if it fixes #3079733: Nested paragraphs not translatable, but it doesn't. Now the fields to translate appear, but the widget shows "(all languages)" which is not correct for the fields in nested paragraphs so it doesn't seem to inherit the correct parent language. See the issue for details on the configuration. Perhaps a look at the paragraphs issue #2864682: Sub-paragraph fields cannot be translated is also helpful.
I thinks all regular multilanguage combinations including nesting, which is typical for many cases, should be tested to work before this is RTBC'ed. Thank you all so much for this promising work and this wonderful module!
Comment #28
itamair CreditAttribution: itamair as a volunteer commentedthe #26 patch doesn't apply cleanly (to me) to the actual dev HEAD (3fd9f5ace4c32134b23f5f384b0beb95afd637a9)
Comment #29
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedThanks @itamair. The attached patch should be up to date with current dev HEAD.
Comment #30
BramDriesenAccording to #27 it's still not ready I think :)
Comment #31
BramDriesenAny update Justin? :)
Comment #32
saschaeggiThe latest patch from #29 removes some functionality so it doesn't apply correctly.
Comment #33
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedThanks @saschaeggi -- what functionality are you referring to?
@BramDriesen and @Anybody -- thanks for testing translations on this. Question: does this patch remove translation functionality that previously was working (meaning *was* work with IEF, but is *not* working with this patch)? If not, I'd like to get this pushed to dev and then track the translation issue separately.
Let me know what you think!
Comment #34
saschaeggi@justin2pin classes are the old ones again, missing paragraphs icons from dev build
Comment #35
justin2pin CreditAttribution: justin2pin at Aten Design Group commented@saschaeggi - thanks and good catch re:icons, I'll get those in.
What do you mean by "classes are the old ones again" though?
Comment #36
saschaeggi@justin2pin I was wrong, I just checked it again. Looks like the CSS classes used after applying the patch are according to BEM.
e.g.
After applying the path
.erl-add-content__group
Dev version:
.erl-add-content--group
So that's very nice. I tested it once again and the only thing (as mentioned before) are the paragraph icons which are missing now.
Maybe we can go with a nicer implementation for the selection of the elements after this. I was exploring it already a bit in this ticket here: 3070005: Implement a nicer solution to element selection
Let me know when you've fixed the icons, so I can test again :-)
Cheers
Comment #37
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedThanks again @saschaeggi -- The attached patch adds support for paragraph icons.
Comment #38
saschaeggi@justin2pin the patch from #37 works like a charm for me.
Comment #39
saschaeggiComment #40
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedJust pushed to dev. Thanks everybody for the help on this... this is a big step for this module.
Also: attached is the final patch for the pushed change. I removed the referenced requirement to IEF as well as the PATCHES.txt file generated locally by composer.
Comment #41
itamair CreditAttribution: itamair as a volunteer commentedEhi ... I haven't got the evidence that this have been pushed into dev yet, isn't it?
Be sure to add the below preset message into the patch porting commit, or at least the 'Issue #3068270 ... ' issue reference.
Comment #42
itamair CreditAttribution: itamair as a volunteer commentedComment #43
itamair CreditAttribution: itamair as a volunteer commentedand also hide old (overcome) patches ...
Comment #44
itamair CreditAttribution: itamair as a volunteer commentedJust to be and make it more clear, also to others maintainers and developers that clone the GIT dev repository, once an issue is closed and tagged as Fixed as a consequence of the porting of the best packed patch, its commit message should be populated with the "Issue #number ... "
to give a clear reference of the commit itself and the issue that generated it.
In this case it would be (as preset here under in the "Credit and committing" section:
This will add a clear mention and link (to the commit) in the issue thread itself, and would give a clear reference of the issue that originated that commit in the Repo Commits history.
In same cases more than commit might be tagged with the same "Issue #number ... " if all incremental amends related to the same issue.
Comment #45
justin2pin CreditAttribution: justin2pin at Aten Design Group commentedThanks @itamair -- my mistake! I had been working on this in a separate branch and and just merged my work without actually applying that latest patch. Will make make sure to just apply the patch and use the right commit message in the future :).