- Follow the example of the text input widget, see \Drupal\typed_data\Plugin\TypedDataFormWidget\TextInputWidget and \Drupal\Tests\typed_data\Functional\TypedDataFormWidget\TextInputWidgetTest
- Add a plugin with id "textarea" which exposes the "rows" option as configuration setting.
- Add a test case

Comments

fago created an issue. See original summary.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Issue tags: +Contributor
max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented
Assigned: Unassigned » max-kuzomko
max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented

Started to work on the last one. Created a pull requests but some tests failed (I think some unit tests and coding standards issues).
I fixed some coding standards issues (errors only).

Could somebody review the PR (remaining coding standards issues + unit tests), have a look at my implementation of textarea widget and give some comments, please?
https://github.com/fago/typed_data/pull/2

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Active » Needs work

thx, that looks pretty solid. There is just one small issue which we should improve a bit imo: https://github.com/fago/typed_data/pull/2#pullrequestreview-40382802

freelock’s picture

freelock
Location Seattle
CreditAttribution: freelock at Freelock commented

Does this support using text formats/filters?

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented

#5, thanks! Fixed!

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented
Status: Needs work » Needs review
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented

We've still some coding style issues pending - we need to resolve them before we can proceed here with a green build.

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented

All checks have passed, please have a look:
https://github.com/fago/typed_data/pull/5

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs review » Needs work

thx, that's quite good already, however there is still the issue of applicable data types. Please see https://github.com/fago/typed_data/pull/2#pullrequestreview-48899068

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko commented
Status: Needs work » Needs review

Fixed, please review

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs review » Needs work

Thank you, however this needs some work still. See PR.

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko as a volunteer commented
Status: Needs work » Needs review

Thanks @fago.

Please review the updated PR:
https://github.com/fago/typed_data/pull/2
Also there is an answer to your comment:
https://github.com/fago/typed_data/pull/2#discussion_r142030238

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs review » Needs work 
Should we have the following testIsApplicable() ?

public function testIsApplicable() {
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('any')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('binary')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('boolean')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('datetime_iso8601')));;
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('duration_iso8601')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('email')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('float')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('integer')));
$this->assertTrue($this->widget->isApplicable(DataDefinition::create('string')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('timespan')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('timestamp')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('uri')));
$this->assertFalse($this->widget->isApplicable(ListDataDefinition::create('string')));
$this->assertFalse($this->widget->isApplicable(MapDataDefinition::create()));
}

Answered at github, but yeah - exactly this!

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko as a volunteer commented

@fago, tests failed:

1) Drupal\Tests\typed_data\Functional\TypedDataFormWidget\TextareaWidgetTest::testIsApplicable
Failed asserting that false is true.
/home/travis/build/fago/drupal/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:31
/home/travis/build/fago/typed_data/tests/src/Functional/TypedDataFormWidget/TextareaWidgetTest.php:64

src\Plugin\TypedDataFormWidget\TextareaWidget.php:

public function isApplicable(DataDefinitionInterface $definition) {
    return is_subclass_of($definition->getClass(), StringInterface::class);
  }

Should we do:

return is_subclass_of($definition->getClass(), StringInterface::class) ||
      is_subclass_of($definition->getClass(), IntegerInterface::class) ||
      is_subclass_of($definition->getClass(), FloatInterface::class);

instead:
return is_subclass_of($definition->getClass(), StringInterface::class);

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented

sure, how else could you make it work as desired? ;)

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko as a volunteer commented
Status: Needs work » Needs review

Thanks! I have updated the PR:
https://github.com/fago/typed_data/pull/2
Please have a look.

Hoverwer, we had a discussion regarding float & integer earlier:
https://github.com/fago/typed_data/pull/2#discussion_r118576386

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs review » Needs work

>Hoverwer, we had a discussion regarding float & integer earlier:
https://github.com/fago/typed_data/pull/2#discussion_r118576386

Exactly.

isApplicable and its test case still needs work. As written before, it should have that test:

public function testIsApplicable() {
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('any')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('binary')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('boolean')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('datetime_iso8601')));;
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('duration_iso8601')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('email')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('float')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('integer')));
$this->assertTrue($this->widget->isApplicable(DataDefinition::create('string')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('timespan')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('timestamp')));
$this->assertFalse($this->widget->isApplicable(DataDefinition::create('uri')));
$this->assertFalse($this->widget->isApplicable(ListDataDefinition::create('string')));
$this->assertFalse($this->widget->isApplicable(MapDataDefinition::create()));
}

The implementation needs to be adapted as needed (only allow strings) to follow that desired behaviour!

max-kuzomko’s picture

max-kuzomko CreditAttribution: max-kuzomko as a volunteer commented

#19, with the specified testIsApplicable() method tests fails:

1) Drupal\Tests\typed_data\Functional\TypedDataFormWidget\TextareaWidgetTest::testIsApplicable
Failed asserting that true is false.
/home/travis/build/fago/drupal/core/tests/Drupal/KernelTests/AssertLegacyTrait.php:43
/home/travis/build/fago/typed_data/tests/src/Functional/TypedDataFormWidget/TextareaWidgetTest.php:61

Since isApplicable() returns TRUE for the following data types:
datetime_iso8601, uri, duration_iso8601

@fago, should allow the data types above for the textarea widget:

$this->assertTrue($this->widget->isApplicable(DataDefinition::create('datetime_iso8601')));;
$this->assertTrue($this->widget->isApplicable(DataDefinition::create('duration_iso8601')));
$this->assertTrue($this->widget->isApplicable(DataDefinition::create('uri')));

or we have to do not allow it by updating isApplicable() method with something like this:

public function isApplicable(DataDefinitionInterface $definition) {
    return is_subclass_of($definition->getClass(), StringInterface::class) && !in_array($definition->getDataType(), ['datetime_iso8601', 'duration_iso8601', 'uri']);
  }

  • fago committed ad6c457 on issue-2871403-2 
    Issue #2871403 by max-kuzomko: Add a textarea input widget.

Squashed...

  • fago authored f5bfe71 on 8.x-1.x 
    Issue #2871403 by max-kuzomko: Add a textarea input widget.

Squashed...
fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented

I just completed the remaining part here + merged it! Thanks!

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago at drunomics commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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