Problem/Motivation

Both "widgets" duplicate a LOT of code. Making it seemingly impossible to effectively work on it without fear of major regressions.

Proposed resolution

Make InlineParagraphsWidget a subclass of ParagraphsWidget and simply reuse existing methods that are the same between the two.

Also, create an additional helper class, like ParagraphsWidgetItem or something to wrap a lot of the boilerplate code so it doesn't have to be determined again in the various methods called (plus it will give constraints).

Remaining tasks

  • Create a patch

User interface changes

None

API changes

Technically making InlineParagraphsWidget a subclass of ParagraphsWidget is a "change", however no actual code/logic would change.

Data model changes

None

CommentFileSizeAuthor
#45 refactor_field_widget-2854444-45.patch31.31 KBpivica
#45 interdiff-2854444-42-45.txt1.12 KBpivica
#45 refactor_field_widget-2854444-44.patch31.89 KBpivica
#43 refactor_field_widget-2854444-43.patch47.49 KBpivica
#43 interdiff-2854444-42-43.txt546 bytespivica
#42 refactor_field_widget-2854444-42-interdiff.txt3.74 KBBerdir
#42 refactor_field_widget-2854444-42.patch31.98 KBBerdir
#39 refactor_field_widget-2854444-39.patch28.31 KBpivica
#39 interdiff-2854444-36-39.txt8.62 KBpivica
#36 refactor_field_widget-2854444-36.patch31.24 KBpivica
#36 interdiff-2854444-30-36.txt647 bytespivica
#31 refactor_field_widget-2854444-30.patch31.19 KBpivica
#30 refactor_field_widget-2854444-30.patch0 bytespivica
#30 interdiff-2854444-27-30.txt38.59 KBpivica
#27 refactor_field_widget-2854444-27.patch59.41 KBpivica
#27 interdiff-2854444-24-27.txt38.97 KBpivica
#24 refactor_field_widget-2854444-24.patch61.4 KBpivica
#24 interdiff-2854444-17-24.txt38.61 KBpivica
#17 refactor_field_widget-2854444-17.patch24.81 KBpivica
#16 Edit and collapse button open.png11.67 KBtoncic
#16 Edit and collapse button.png7.9 KBtoncic
#16 Lockable and collapse button.png4.84 KBtoncic
#15 Locked and collapse button.png9.08 KBtoncic
#15 Edit and collapse button open.png46.6 KBtoncic
#15 Edit and colapse button.png12.3 KBtoncic
#4 refactor_field_widget-2854444-4.patch74.15 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drobnjak created an issue. See original summary.

markhalliwell’s picture

Status: Active » Closed (duplicate)

This is just duplicating that existing issue.

markhalliwell’s picture

Title: Remove dropdown button markup from classical widget » Refactor field widget code to reduce duplication
Assigned: drobnjak » markhalliwell
Category: Bug report » Task
Issue summary: View changes
Status: Closed (duplicate) » Active

Actually, I'll just re-purpose this issue instead of creating a new one.

markhalliwell’s picture

Initial start. This gets at least the dropbuttons to render properly. Ajax mostly works, although some of the paragraph item operations need double checking.

This is all still a WIP (tests will likely fail) as a lot of the logic needs to still be flushed out... so setting status appropriately and leaving it assigned to me.

I'll try to follow-up this weekend.

miro_dietiker’s picture

Thx for starting this!

We already removed a lot of redundancy or even dead code and yeah there's a long way to go.

Please focus on the new experimental widget, where we fear a bit less to break things and we are also fine to remove certain settings / variants if the default experience is satisfying. We don't want to risk breaking the old Paragraphs Classic widget.

I also updated the issue about the collapsible element.
Please mind that initially this edit + expansible button looks like a fixed combination. But there will be cases where we have no default action and only the expand button. I don't think the core dropbutton can be used that way.

The item looks a bit like like a monster with all its duplicated local properties.

miro_dietiker’s picture

Also one final note,

We need to be careful not to waste work due to overlap with other ongoing work in other issues.

markhalliwell’s picture

Thx for starting this!

My pleasure. It's really a nifty module :D

Please focus on the new experimental widget

That is the initial goal, yes.

We don't want to risk breaking the old Paragraphs Classic widget.

I don't plan on touching the classing plugin (much). I don't want to risk breaking anything there either. The only thing I currently did is add a special class to that so it can be targeted in CSS.

Please mind that initially this edit + expansible button looks like a fixed combination. But there will be cases where we have no default action and only the expand button. I don't think the core dropbutton can be used that way.

The patch above now handles single or multiple buttons and converts them into dropbuttons automatically as needed.

The item looks a bit like like a monster with all its duplicated local properties.

I'm assuming you're referring to ParagraphsWidgetItem. It's actually reducing duplication. With all the different methods in ParagraphsWidget, a lot of them have to recreate these same properties anyway. By abstracting this into a dedicated class we gain a lot here, specifically:

  • Dependency Injection - a single object (see the new ParagraphsWidget::buildParagraphItemMessages and ParagraphsWidget::buildParagraphItemOperations methods) rather than having to pass all the different variables to construct the same information.
  • Performance - only one object is created and then initialized. Once these properties are set, we can reuse them where ever they're needed.
  • Typed data - autocompletion/documentation, it's not just random variables

I should also point out that this new class isn't fully flushed out either, there's a lot more than can be abstracted here.

markhalliwell’s picture

I also meant to mention that even if the classic plugin inherits from the new one, it can still override all the methods it needs to provide all the original functionality.

The main reason it needs to inherit though is so it can use all the new helper methods (e.g. ParagraphsWidget::buildButton() and ParagraphsWidget::buildDropbutton()) to properly construct the elements it needs to.

It can still keep the same logic/buttons/styles etc, but with better and more stable theme/render array construction using proper APIs ;)

markhalliwell’s picture

We need to be careful not to waste work due to overlap with other ongoing work in other issues.

I agree, however, I've seen these UI/UX issues (also following) and I have to be honest... many have some pretty significant issues. If only because they're attempting to create new concepts instead of working with existing core concepts/APIs.

I'm all for design, but I think there's a happy medium that can be achieved so that it's not built in a way that "assumes" Seven is the theme being used.

I also have a sneaky suspicion that once this issue is in, there will be a better foundation for moving forward in all of those issues.

miro_dietiker’s picture

That sounds great. I'm also triggering some of our team to review the intermediate state.
I didn't yet really find time to review the code.

The new widget is here to get unlimited freedom in doing things better and different.
Some of my proposals have not yet been properly adapted or possibly not fully understood. They're not design proposals. They're usability proposals to work around core UI pattern limitations that lead to bad UX. The problems all look too simple if you don't look at advanced use cases where paragraphs combines all the available things. That's why we push hard on Paragraphs Collection with example content.

Extending one from the other or the other way round: I don't even want to think about when we risk breaking appearance of the classic widget or what types of subclassing / method overrides of custom widgets based on the old code we break. We simply started with a clone, but i guess it won't take that long until it's going to be a much different thing in all aspects.

Thank you for helping us to move forward with Paragraphs.

pivica’s picture

Just a quick ping from me, started reviewing this issue and current patch because its on our blocker list now. @markcarver if you have more thoughts on current patch please share. I will provide new patch that works with latest version as soon as possible and we can continue from there.

markhalliwell’s picture

I'll try and look at this again while at DC next week.

miro_dietiker’s picture

Quickly discussed how to review and move forward here best and we have here at least 5 different subject mixed:
- certain non-functional codestyle issues (indentation, array( conversion)
- CSS selectors that seem to be cleaned, without functional impact
- introduce createMessage() / buildParagraphItemMessages()
- introduction of a ParagraphWidgetItem / widget state / $paragraph_item
- introduce a Paragraph Operation class + buildButton() + buildDropButton()

IMHO the first 3 would have been easy to separate and quickly commit.
We will need to decide if the Item / state abstraction will help us with maintenance. Proof could be that a separate patch, reduces overall code...

Are you saying that we can only do the last Operation class efficiently, if we have ParagraphWidgetItem transition applied?

I would propose that after a reroll, we work through the hunks and drop the first 3 changes. And/or outsource them to a separate issue, quickly commit after some user testing, and reroll here.

markhalliwell’s picture

Title: Refactor field widget code to reduce duplication » Refactor field widget code to reduce duplication and make theme abstract

- certain non-functional codestyle issues (indentation, array( conversion)

This is just automatic per IDE considering recent adoption of core coding styles, sorry.

- CSS selectors that seem to be cleaned, without functional impact

This was primarily because it's currently just using default field provided class names, not dedicated classes provided by this module. This can change in themes and was necessary to get some of this working with Bootstrap (or any theme really).

- introduce createMessage() / buildParagraphItemMessages()
- introduction of a ParagraphWidgetItem / widget state / $paragraph_item
- introduce a Paragraph Operation class + buildButton() + buildDropButton()

These need to stay in this issue as it pertains to what this entire issue is about. Plus, like I said above, this is still a WIP and not yet fully complete.

---

Part of why all this is together is because I was still working on everything and wasn't quite sure how and what all needed to be changed (that happens in a refactor).

The reality is:

This issue is about making this module theme abstract so it will work in any theme.

This is similar to what I did for #2803407: Refactor form element and JS to work with any theme. Granted, that was for their JS, but the concept still remains the same.

Proper theming often impacts large chunks of code because of how convoluted and interdependent core's entire theme system is.

Thus, a lot of this stuff is intertwined just to get proper dropbuttons (using core's native APIs) working.

My goal has always been to create issues/patches that, when applied, doesn't break anything.

The suggestions above regarding splitting out some of this functionality makes some of this impossible.

I will gladly split out non-related (to this issue) code changes into separate issues once this is done.

Until then, I'm not going to create several different issues that I'll have to track or keep up with when this issue is still currently up in the air.

toncic’s picture

Just to be more clear, here is a few different cases how action buttons should looks.

toncic’s picture

Embedding for better readability.

pivica’s picture

Regarding comment 15 and 16 this mockups are still work in the progress and they should not be mixed with this issue, the issue for mockups is is currently #2851672-7: Display icons in collapsible action button actions.

So after lots of trying to review this issue and back and forward things here is finally new version that is tested and seems it is working fine. Previous patch is broken and can not be applied cleanly so difficult to create proper interdiff and also there are a lot of changes. I will try to summarize most important ones here:

  • All proposed code style error changes are removed. Changes are fine but we should do this in some follow up issue and not here.
  • Proposed CSS changes around adding more stable class names, which i think are a good idea, are moved to follow up issue #2875862: Use custom class for identification so we can concentrate here just on PHP widget code cleanup.
  • I removed proposed ParagraphsTypeInterface for now - was not quite sure about re-usability of that code for now, i think if a need for this arise in future we should do it then and tryo to avoide it now.
  • Proposed ParagraphOperations class, not sure about this, but it looks like a cool idea, especially because we will need a way to extend how default dropdown works in the future and this class is hopefully allowing as that. So i leaved this for now.
  • The biggest changes are in ParagraphsWidget class
    • I've removed proposed $widgetState variable, seems to me that it is not doing anything really useful for now.
    • Removed proposed buildParagraphItemMessages method - that code logic is still in formElement method. Same applies to buildParagraphItemOperations method - that code is also still in formElement method.
    • Proposed createMessage method is still here, i've just simplified it a bit.
    • I've keept proposed buildButton and buildDropbutton. Did some minor adjustment in buildButton method.
    • Added paragraphs_widget variable for hook_alter_paragraph_widget_dropbutton implementation so alters can use buildButton method. We need this for test and also for paragraphs_library_paragraph_widget_dropbutton_alter() implementation - follow up #2875857: Use new buildButton method from ParagraphsWidget.
    • What is cool is that we still use code from markcarver that removed custom hardcoded html wrappers and classes that was manually creating drop button.

@markcarver yeah this drops maybe half of your proposals but please take into consideration that the bigger the patch/changes the harder is for everybody to do a review of it. We should try to avoid this situation so changes can be applied quicker. Please when possible try to separate patches into smaller follow ups so its easier for review and things will hopefully get merged faster.

pivica’s picture

Status: Needs work » Needs review

And lets try tests...

The last submitted patch, 4: refactor_field_widget-2854444-4.patch, failed testing.

The last submitted patch, 4: refactor_field_widget-2854444-4.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: refactor_field_widget-2854444-17.patch, failed testing.

markhalliwell’s picture

Assigned: markhalliwell » Unassigned

Please when possible try to separate patches into smaller follow ups so its easier for review and things will hopefully get merged faster.

Then please, when possible:

  1. Don't duplicate code/classes when they're nearly identical.
  2. Use proper core APIs instead of manual markup.
  3. Try to develop code in a theme abstract way instead of just assuming that the markup/DOM hierarchy/classes/js provided by core will "always be the same" in every theme.
  4. Don't condescend people who are simply trying to help. You'll likely push them away, as you now have with me.

But yes, I will be sure to take your advice and try to make a "better effort" in the future. I just have to remember that if I start theme abstracting code in a project, the proper way, to actually finish it before uploading what I had already stated was just the initial [WIP] patch. That way, people won't start picking it apart just because they cannot see the larger picture.

It's clear with as much as I have going on right now, I'm not going to be able to get to this as I had planned to. Quite frankly, I also don't have the energy to continue arguing about this anymore.

It doesn't seem that my comments on explaining why all these changes are interconnected and necessary when dealing with theme abstracting code (aside from the IDE array changes, which are... whatever, get over it).

I guess just continue to do whatever you want instead of taking the advice and efforts of someone who truly understand's core's theme system.

Whatever.

Unassigning myself.

pivica’s picture

Hey @markcarver there is no need to take this on such a personal level, things like 'please try to separate patches' is a constructive code/managment request because it will be much more easier and faster for other people to review them - meaning we will be able to roll things faster and not slower.

> Then please, when possible:
> 1.
> 2.
> 3.

Yeah i totally agree, but we are not living in ideal world and working condition. Current code was written by many people (juniors and seniors) over the many months and lots of code were written to quickly explore concepts and the code is not in final state. And sure some parts are the mess, nobody is saying code is proper ;)

However current code is already used on other places. There are ton of tests and other modules and business projects that are depending on it. We would all be very happy if we would have a budget and time to stop all we are doing and spent next one month or more fully concentrate on cleaning current code and make it more abstract and better and then restart other operations, but sadly this is not the case.

> 4. Don't condescend people who are simply trying to help. You'll likely push them away, as you now have with me.

Nobody is pushing you away, you are more then welcome to propose more additional changes and concept and show us why they are good and if they are working in real business cases everybody will be happy to accept them and use them.

But please understand that if you are proposing a big changes that for sure other will complain because a lot of effort needs to be invested in reviews, rewriting tests, doing manual tests, testing in running projects, adjusting running project tests, etc... so yeah we will need to invest a lot of energy and sure we will not be happy with it ;)
This is the only reason we are suggesting you to split your changes in smaller chunks that are easier to manage/review.

But I also understand your wish to do more and do it in one big push then doing a lots of smaller push. I also often need to restrain my self in doing the same thing and then produce all above management problems i wrote above to other people. So if you are really so unhappy with our request to split changes in smaller chunks then as alternative way why don't you extend a new ParagraphsWidget class in a new follow up issue, name it differently and do big changes there which are not affecting current system. You can then work as you like, do all the big changes you want, do with your rhythm and when the thing are ready then you ping us to start reviewing/testing the stuff.

pivica’s picture

Status: Needs review » Needs work

The last submitted patch, 24: refactor_field_widget-2854444-24.patch, failed testing.

Berdir’s picture

  1. +++ b/modules/paragraphs_demo/src/Tests/ParagraphsDemoTest.php
    @@ -164,7 +164,7 @@ class ParagraphsDemoTest extends WebTestBase {
         $this->drupalGet('node/1/edit');
    -    $this->assertFieldByName('field_paragraphs_demo_3_subform_field_paragraphs_demo_0_duplicate');
    +    $this->assertFieldByName('field-paragraphs-demo-3-duplicate');
    

    This is not the same thing. This was specifically asserting that also nested paragraphs show the duplicate button.

  2. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -279,11 +279,7 @@ class ParagraphsWidget extends WidgetBase {
    -        $info['validation_error'] = array(
    -          '#type' => 'container',
    -          '#markup' => $this->t('@messages', ['@messages' => strip_tags(implode('\n', $messages))]),
    -          '#attributes' => ['class' => ['messages', 'messages--warning']],
    -        );
    +        $info['validation_error'] = $this->createMessage($this->t('@messages', ['@messages' => strip_tags(implode('\n', $messages))]));
    

    this t() makes no sense, there's nothing to translate. It was there before, so probably separate issue.

  3. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -382,89 +378,55 @@ class ParagraphsWidget extends WidgetBase {
    +        $links += $this->buildButton(Html::getId($id_prefix . '_duplicate'), $this->t('Duplicate'),
    +          [self::class, 'duplicateSubmit'], [self::class, 'itemAjax'], $widget_state['ajax_wrapper_id'], [
    +            '#access' => $paragraphs_entity->access('update'),
                 '#delta' => $delta,
    -            '#ajax' => [
    

    This doesn't really become more readable with all those arguments.

    Yes, it was a bit long before, but it was also visible what was an argument for what.

    When we refactored the add button code, we spent quite some time on avoding a large number of arguments.

    Ene example would be to instead of passing the additional array keys in as an argument would be to append them as buildButton(...) + [...]. Some arguments could then already have default values, like the ajax callback.

    Haven't seen the method yet. Yet another option would be to just keep defining the buttons and then just loop over all links and call something like expandButton($link) or something, then we can just leave out al the generic stuff.

    It would also more or less just work for operations added through the hook.

  4. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -382,89 +378,55 @@ class ParagraphsWidget extends WidgetBase {
    -          $info['edit_button_info'] = array(
    -            '#type' => 'container',
    -            '#markup' => $this->t('You are not allowed to edit this @title.', array('@title' => $this->getSetting('title'))),
    -            '#attributes' => ['class' => ['messages', 'messages--warning']],
    -            '#access' => !$paragraphs_entity->access('update') && $paragraphs_entity->access('delete'),
    +          $info['edit_button_info'] = $this->createMessage(
    +            $this->t('You are not allowed to edit this @title.', array('@title' => $this->getSetting('title'))),
    +            !$paragraphs_entity->access('update') && $paragraphs_entity->access('delete')
               );
    

    similar here, I'm really not that convinced that createMessage() adds that much value. we save 1-2 lines, but have non-named arguments to a method now, and I have to understand the code to know that the second argument is the access check. I'm also not a fan of multi-line method arguments there are afaik no clear coding standard guidelines other than that they should be avoided if possible.

    So again here, my suggestion would be to leave the #access out of the message thing and add it with + [#access => ...], formatted like this:

    $this->createMessage(....) + [
    #access => ...,
    ];

    IMHO that is more readable and shows what

  5. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -1029,58 +949,142 @@ class ParagraphsWidget extends WidgetBase {
    +        'progress' => ['type' => 'fullscreen'],
    

    this is interesting, didn't think I've seen this used before. Was added in #2207695: Expand the throbber API to include a 'full screen' option and core only uses it for views and outside in.

  6. +++ b/src/Tests/Experimental/ParagraphsExperimentalAccessTest.php
    @@ -133,9 +133,9 @@ class ParagraphsExperimentalAccessTest extends ParagraphsExperimentalTestBase {
         // Check the remove button is present.
    -    $this->assertNotNull($this->xpath('//*[@name="field_paragraphs_demo_0_remove"]'));
    +    $this->assertNotNull($this->xpath('//*[@name="field-paragraphs-demo-0-remove"]'));
    

    are we sure about this rename? this means it is the actual form element name that is using - now instead of _, that's very uncommon?

    It also causes a ton of test changes that would otherwise not be needed.

pivica’s picture

Okey new version ready.

> 1.

Fixed, sorry for that.

> 2.

Does this means that messages that are coming from ConstraintViolationInterface::getMessage() are already translated?

> 3.

I've added new ::expandButton() method for now for this.

> 4.

Done.

> 5.

Seems its working better then with default blue inline throbber, i like it.

> 6.

Not sure about this really, @markcarver any idea why we are doing this in original patch?

For now this code is isolated in ParagraphsWidget::expandButton() with some comments and @todo and commented code that is returning '-' to '_'. I am in favor that we return back to '_' but lets wait a little bit if @markcarver will reply.

In worst case i will uncomment strtr function and update the tests one more time.

Status: Needs review » Needs work

The last submitted patch, 27: refactor_field_widget-2854444-27.patch, failed testing.

Berdir’s picture

+++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
@@ -378,56 +378,63 @@ class ParagraphsWidget extends WidgetBase {
+          $info['edit_button_info'] = $this->createMessage($this->t('You are not allowed to edit this @title.', array('@title' => $this->getSetting('title')))) + ['#access' => !$paragraphs_entity->access('update') && $paragraphs_entity->access('delete')];

I'd wrap the appended array on a new line as suggested:
$this->createMessage() + [
#access
];

then it's not quite as long.

pivica’s picture

> I'd wrap the appended array on a new line as suggested:
> $this->createMessage() + [

Right its shorter and nicer, somehow i understood you that you want other way around without that new line...

Also discussed with Miro about '-' char problem in name attribute - we both agreed that without knowing exactly why would we put '-' char instead of '_' in name attributes its better to fallback to the way how it was before. So reverted that part also and updated tests.

pivica’s picture

pivica’s picture

> Also discussed with Miro about '-' char problem in name attribute - we both agreed that without knowing exactly why would we put '-' char instead of '_'

And yeah @markcarver when you have time to give us feedback for this do we need '-' in name attribute please do.

The last submitted patch, 30: refactor_field_widget-2854444-30.patch, failed testing.

The last submitted patch, 30: refactor_field_widget-2854444-30.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: refactor_field_widget-2854444-30.patch, failed testing.

pivica’s picture

Status: Needs review » Needs work

The last submitted patch, 36: refactor_field_widget-2854444-36.patch, failed testing.

Berdir’s picture

  1. +++ b/css/paragraphs.widget.css
    @@ -115,7 +118,3 @@
    -
    -.paragraphs-tabs-wrapper .paragraphs-tabs {
    -  display: none;
    -}
    

    is this really something we want to do here? we did that on purpose, so we only show those things with JS when there is something to show. sure this isn't some kind of merge conflict? Or maybe it was not added to sass file?

  2. +++ b/modules/paragraphs_demo/src/Tests/ParagraphsDemoTest.php
    @@ -164,7 +164,7 @@ class ParagraphsDemoTest extends WebTestBase {
         $this->drupalGet('node/1/edit');
    -    $this->assertFieldByName('field_paragraphs_demo_3_subform_field_paragraphs_demo_0_duplicate');
    +    $this->assertFieldByName('field-paragraphs-demo-3-subform-field-paragraphs-demo-0-duplicate');
       }
    

    this still looks for "-"?

  3. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -279,11 +279,7 @@ class ParagraphsWidget extends WidgetBase {
    +        $info['validation_error'] = $this->createMessage($this->t('@messages', ['@messages' => strip_tags(implode('\n', $messages))]));
    

    to answer your question. Yes, validation messages are translated already. But this doesn't actually translate anything anyway. It just passes everything through a single placeholder, that can't be translated. But as I said, lets do a separate issue to clean this up.

  4. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
    @@ -382,10 +378,10 @@ class ParagraphsWidget extends WidgetBase {
     
    -        $links['duplicate_button'] = [
    +        $links['duplicate_button'] = $this->expandButton([
    

    My idea was something like this:

    You generate $links normally, like before, except without the standard stuff that the expandButton() method does.

    Then, after the alter hook, you call $links = array_map([$this, 'expandButton'], $links)

    Then all links, including those from the alter hook will automatically be expanded and there are only minimal changes necessary here and in those hook implementations.

  5. +++ b/src/Tests/Experimental/ParagraphsExperimentalDuplicateFeatureTest.php
    @@ -28,7 +28,7 @@ class ParagraphsExperimentalDuplicateFeatureTest extends ParagraphsExperimentalT
         $this->addParagraphsType($paragraph_type);
    -    $this->addParagraphsType('text');
    +//    $this->addParagraphsType('text');
     
    

    why is this no longer needed? if really not needed anymore, then just remove it.

  6. +++ b/src/Tests/Experimental/ParagraphsExperimentalSummaryFormatterTest.php
    @@ -34,11 +34,10 @@ class ParagraphsExperimentalSummaryFormatterTest extends ParagraphsExperimentalT
     
    -    // Add a user Paragraph Type
    +    // Add a user Paragraph Type.
         $paragraph_type = 'user_paragraph';
         $this->addParagraphsType($paragraph_type);
         static::fieldUIAddNewField('admin/structure/paragraphs_type/' . $paragraph_type, 'user', 'User', 'entity_reference', ['settings[target_type]' => 'user'], []);
    @@ -47,11 +46,10 @@ class ParagraphsExperimentalSummaryFormatterTest extends ParagraphsExperimentalT
    
    @@ -47,11 +46,10 @@ class ParagraphsExperimentalSummaryFormatterTest extends ParagraphsExperimentalT
         $this->drupalGet('admin/structure/types/manage/paragraphed_test/display');
         $edit = ['fields[field_paragraphs][type]' => 'paragraph_summary'];
         $this->drupalPostForm(NULL, $edit, t('Save'));
    -    // Add a paragraph.
    +
    +    // Create a node with a title and a text.
         $this->drupalPostAjaxForm('node/add/paragraphed_test', [], 'field_paragraphs_text_paragraph_add_more');
         $this->drupalPostAjaxForm(NULL, NULL, 'field_paragraphs_title_add_more');
    -
    -    // Create a node with a text.
    

    there are now only some unrelated comment changes left in this file, lets remove that from this patch completely.

  7. +++ b/src/Tests/Experimental/ParagraphsExperimentalTranslationTest.php
    @@ -92,11 +92,10 @@ class ParagraphsExperimentalTranslationTest extends ParagraphsExperimentalTestBa
         $this->clickLink(t('Edit'));
     
    -    $this->drupalPostAjaxForm(NULL, NULL, 'field_paragraphs_demo_nested_paragraph_add_more');
    -    $this->drupalPostAjaxForm(NULL, NULL, 'field_paragraphs_demo_1_subform_field_paragraphs_demo_text_add_more');
    +    $this->drupalPostAjaxForm(NULL, NULL, 'field_paragraphs_demo_text_add_more');
         $edit = [
           'field_paragraphs_demo[0][subform][status][value]' => FALSE,
    -      'field_paragraphs_demo[1][subform][field_paragraphs_demo][0][subform][field_text_demo][0][value]' => 'Dummy text'
    +      'field_paragraphs_demo[1][subform][field_text_demo][0][value]' => 'Dummy text'
         ];
    

    this change is likely also no longer required and it's also different, we were specifically testing nested paragraphs before.

  8. +++ b/src/Tests/Experimental/ParagraphsExperimentalWidgetButtonsTest.php
    @@ -174,11 +174,11 @@ class ParagraphsExperimentalWidgetButtonsTest extends ParagraphsExperimentalTest
         $this->drupalGet('node/add/paragraphed_test');
         $this->drupalPostForm(NULL, NULL, t('Add text'));
    -    $this->assertNoField('edit-field-paragraphs-0-top-links-test-button');
    +    $this->assertNoField('field-paragraphs-test');
         \Drupal::state()->set('paragraphs_test_dropbutton', TRUE);
         $this->drupalGet('node/add/paragraphed_test');
         $this->drupalPostForm(NULL, NULL, t('Add text'));
    -    $this->assertField('edit-field-paragraphs-0-top-links-test-button');
    +    $this->assertField('field-paragraphs-test');
    

    also here, is this really the same thing?

    I think we were testing to an additional button through a test module?

pivica’s picture

> 1.

This came from before - change was not done in SASS but just in CSS and recompilation removed it. That code is added now in SASS also.

> 2.

Fixed.

> 3.

Follow up issue on https://www.drupal.org/node/2877351.

> 4.

Not sure about this, there are some cases where we are calling extendButton for item that is not in dropdown button like

$element['top']['paragraphs_edit_button_container']['paragraphs_edit_button'] => $this->expandButton(...)

How will we then approach to this case, i guess leaving expandButton call there is fine?

But the original idea yeah i figure what you want and its cool :) added to the patch.

> 5., 6., 7., 8.

Yeah not needed any more, reverted that stuff.

Status: Needs review » Needs work

The last submitted patch, 39: refactor_field_widget-2854444-39.patch, failed testing.

miro_dietiker’s picture

I guess Berdir proposed 4. implementation.

While on this, i guess i'll commit then, but there are tons of things discussed above and dropped from this issue. All follow-ups need to be created and referenced to this issue. Will set back to needs-work after commit to revisit each previous comment...

Berdir’s picture

This fixes the test .

search api integration was not working, the fail was a negative assertion looking for something that didn't work anyway and something in search_api changed a bit that made it now fail, but it was broken all along anyway like the other already commented out assertions.

#2792277: Respect the "syncing" state when saving an entity was supposed to fix it but didn't actually do that because installing modules is not a config sync.

Fix: Move search api integration config to optional config, ensures it is installed after the fields and then everything is fine.

pivica’s picture

Small addition, we do not need 'paragraphs_widget' in alter context any more.

Berdir’s picture

+++ b/paragraphs.api.php
@@ -39,6 +39,7 @@ function hook_paragraphs_behavior_info_alter(&$paragraphs_behavior) {
  *   - form_state: The current state of the form.
+ *   - paragraphs_widget: The current instance of ParagraphsWidget.
  */

yeah but then you also need to remove it from the docs :)

pivica’s picture

Damn it you are right :)

New patch, also addded gitcofnig renames = copies flag so the patch is smaller.

pivica’s picture

And ignore refactor_field_widget-2854444-44.patch - somehow i uploaded older version with a new one, refactor_field_widget-2854444-45.patch is valid one.

pivica’s picture

And a reply to Miro's comment from 41

Whiile on this, i guess i'll commit then, but there are tons of things discussed above and dropped from this issue. All follow-ups need to be created and referenced to this issue. Will set back to needs-work after commit to revisit each previous comment...

Here is the list of all current follow-ups that are created from this issue:

  • Primsi committed f529860 on 8.x-1.x authored by pivica
    Issue #2854444 by pivica, Berdir, markcarver, toncic, miro_dietiker:...
Primsi’s picture

Priority: Major » Normal
Status: Needs review » Needs work

Committed, many thanks to everyone involved!

As mentioned in #41 moving back to Needs-work for possible additional follow-ups while moving priority to Normal.