Offer a second experimental widget

One thing to consider is test coverage. How are we going to ensure that both the old and the new widget have sufficient test coverage?

How exactly are we going to define the widget as experimental? put it in th label? Make it part of an optional module?

Gonna work on this.

First try, not sure about naming yet. Css cloned.

Tests fixed.

Not sure if that is the best way to do it, neither if it is the right approach but certainly, it works. :)

Fixing tests.

I would change "paragraphs.experimental.css" to "paragraphs.alternative.css" or similar, it would make sense to label it as experimental in the body and labels but it would be best to not use in the file names. Thank you for developing this.

Discussed with @miro_dietiker this approach might be better. I was thinking, what if instead we create methods for all parts of the widget, like buildTop which it can call to buildTitle and buildActions. But it is just a thought.

Fixed tests and changed the widget id's. I really think we should provide a buildTop(). :)

Adding schema. No idea why it passes with php 7. :)

Tests added. :).

Changed array by [].

1. +++ b/css/paragraphs.admin.css
   --- /dev/null
   +++ b/css/paragraphs.experimental.css
   

   one idea to avoid "experimental":

   make separate css files or even libraries,name them after what they're actualy for. So this would be paragraphs.delete.css or something like that.

2. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -0,0 +1,72 @@
   +/**
   + * Plugin implementation of the 'entity_reference_revisions paragraphs' widget.
   + *
   + * @FieldWidget(
   + *   id = "paragraphs",
   + *   label = @Translation("Paragraphs EXPERIMENTAL"),
   + *   description = @Translation("An experimental paragraphs inline form widget."),
   + *   field_types = {
   + *     "entity_reference_revisions"
   + *   }
   + * )
   + */
   +class ParagraphsWidget extends InlineParagraphsWidget {
   

   Hm.

   I was wondering if we should do it like this or if we should just copy the file completely.

   This saves duplicate code now, but everything we want to do here will be more complicated and it will likely result in changes and refactoring of the existing widget.

   I'm not sure if we rather just want to copy it 1:1. One problem that I see test coverage, how do we ensure to not break any existing functionality if we don't run all the test we have with both widgets?

Changed approach, copied file. Still need to work on tests. :)

OK, duplicated most of the tests. Thinking about if we should create a new test environment as diff module has now. That would speed up the tests. :)

Duplicated all tests. :)

Some changes.

Was requested to review this patch.

1. +++ b/src/Plugin/Field/FieldWidget/ParagraphsWidget.php
   @@ -14,32 +14,27 @@ use Drupal\Core\Form\FormStateInterface;
   + *   label = @Translation("Paragraphs EXPERIMENTAL"),
   

   Is this intended to be capitalised (or was it for a personal test) ?

2. +++ b/src/Tests/ParagraphsExperimentalAccessTest.php
   @@ -0,0 +1,146 @@
   +    $admin_user = [
   ...
   +    $this->loginAsAdmin($admin_user);
   ...
   +    $this->loginAsAdmin($admin_user);
   

   This actually is $admin_permissions. You could either give the permissions array as anonymous varable, save $admin_user = $this->loginAsAdmin(...) and then use a normal drupalLogin() below, or just rename this variable :P

3. +++ b/src/Tests/ParagraphsExperimentalAccessTest.php
   @@ -0,0 +1,146 @@
   +    $edit = array(
   +      'settings[uri_scheme]' => 'private',
   +    );
   ...
   +      ->setComponent('field_paragraphs_demo', [
   +        'type' => 'paragraphs',
   +      ]);
   

   If there is just one item and it fits in the 80-char-length rule, I guess you can keep it in one single line.

4. +++ b/src/Tests/ParagraphsExperimentalAddModesTest.php
   @@ -0,0 +1,244 @@
   +    $this->loginAsAdmin();
   +    $this->addParagraphedContentType('paragraphed_test', 'paragraphs_field');
   

   As these two lines, or at least addParagraphedContentType(), (are)/is executed first for each test method, you could add it to setUp() in this class (for every test class).

5. +++ b/src/Tests/ParagraphsExperimentalAddModesTest.php
   @@ -0,0 +1,244 @@
   +    // Use the experimental widget.
   +    $form_display = EntityFormDisplay::load('node.paragraphed_test.default')
   +      ->setComponent('paragraphs_field', [
   +        'type' => 'paragraphs',
   +      ]);
   +    $form_display->save();
   

   This code is done multiple times in the new tests added.
   There is a helper method similar already, setAddMode($content_type, $paragraphs_field, $mode, $experimental = FALSE), that sets 'settings' => ['add_mode' => $mode] also.
   Should we have a more general method with:
   - $id to be loaded ($id seen so far: node.paragraphed_test.default, node.paragraphed_content_demo.default, node.article.default, contact_message.test_contact_form.default)
   - and the component $name to be set ($name seen so far: paragraphs, paragraphs_field, field_paragraphs, field_paragraphs_demo, paragraph.nested_paragraph.default),
   something like setParagraphsFormDisplayComponent($id, $name) in ParagraphsTestBase to reduce more lines everywhere?

6. +++ b/src/Tests/ParagraphsExperimentalAddModesTest.php
   @@ -0,0 +1,244 @@
   +  protected function assertAddButtons($options) {
   ...
   +  protected function assertSelectOptions($options, $paragraphs_field) {
   
   +++ b/src/Tests/ParagraphsExperimentalInlineEntityFormTest.php
   @@ -0,0 +1,174 @@
   +  protected function setParagraphsWidgetMode($content_type, $paragraphs_field, $mode) {
   

   I've seen many asserts (and other kind of helper methods) duplicated (not only because of the cloned experimental X test). Maybe we could move all these helper methods in ParagraphsTestBase ?

7. +++ b/src/Tests/ParagraphsExperimentalTranslationTest.php
   @@ -0,0 +1,856 @@
   +  protected $admin_user;
   

   Unused property.

Not entirely sure if your goal is to then keep these duplicated tests and later extend them for more experimental widget functionality, or it is just a temporary situation and they will be merged in the existing tests. But if these tests will be kept, couldn't we provide helper methods in something like a trait, or better rename the existing testBla to doTestBla and add testBla() { $this->doTestBla(); } and

testBlaExperimental() { // set something for experimental related context
$this->doTestBla(); }

since most of the new tests are adding something at the beginning of each test ?

Ok no, you are going to extend these tests so trying to avoid duplication won't work for later purpose...

+++ b/src/Tests/ParagraphsExperimentalContactTest.php
@@ -0,0 +1,55 @@
+
+namespace Drupal\paragraphs\Tests;
+

What about using a namespace for the tests?

Drupal\paragraphs\Tests\Classic
Drupal\paragraphs\Tests\Experimental

not sure about keeping the experimental in the class name too, but this should be easier to for example do a search and replace only in the new tests?

this should be easier to for example do a search and replace only in the new tests?

I was thinking about it as well.

Ok for duplicated code. But I still think we could provide helper methods to "encapsulate" pieces of tests, like

$this->addParagraphsType('text');
$this->fieldUIAddNewField(...);
$this->assertText(...);

file_put_contents(...);
$image = $this->container->get('file_system')->realpath(...);
$edit = [ ... ];
$this->drupalPostForm(NULL, $edit, 'Update');

...

Since, even without duplicating the tests, some processes are repeated in different tests. Sure it is not related to this issue though.

About naming this experimental widget, if I got it right what differs this from the classic one is that some "labeled buttons" are "reduced" to an icon or some processes (confirm remove, ...) are "reduced", all made for "imagery – guide visual treatments" or similar.
What about something like "Printed-based Design"/"Print Design" or if it is too confusing for an end-user maybe something like "Graphic Print Design"/"Graphic Design" (not sure about this one though)... Got inspired from material design principles explanations.

That's the difference now, we want to make a lot more changes, so I think just sticking with Experimental is fine for now.

What about behaviors? Should we remove that from the classic widget completely as well? Will not be able to provide anything that's even remotely useful for users. Might need a notice then in the paragraph type settings that behaviors are only support with the experimental widget?

Same with helper functions, we could add them for non-widget related things but that's something that can be done later.

Addressed changes from previous comments. Open issues for helper methods and behavior plugins.

Adding per-paragraph-type config tests. :)

OK, so behavior plugins were removed from the classic plugin, tests were split. :)

Added warning message for using behavior plugins with the experimental widget.

Could the front page of this module perhaps explain the difference between 'classic' and 'experimental'? It's a bit mysterious now.