- Status changed to Needs work
almost 2 years ago 9:34am 23 January 2023 - 🇳🇿New Zealand quietone
This time I looked at the code in conjunction with testing the patch and running the tests.
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ +use Drupal\Component\Utility\Xss;
This should be sorted to ease readability.
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ + * label = @Translation("Details with Summary tag"),
The ux folks will give feedback but this should be sentence case.
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ + if ($this->getSetting('open')) {
Why is this shown only when open? As a user I need to click to find the state. Would it not be better to inform the user of the state?
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ + $bubbleable_metadata->applyTo($elements);
In \Drupal\text\Plugin\Field\FieldFormatter\TextDefaultFormatter::viewElements there is a comment that the ProcessedText elements handles the cache. So, I think this is not necessary.
-
+++ b/core/modules/text/config/schema/text.schema.yml @@ -116,6 +116,17 @@ field.formatter.settings.text_trimmed: + label: 'Open details group by default.'
Labels do not have full stops.
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ + // Compute the text to use for the <summary> element.
Let's make this something helpful. Something along the lines of 'Use the user text for the summary. If not supplied then use the field label." This block is in two methods, so both should be changed.
-
+++ b/core/modules/text/src/Plugin/Field/FieldFormatter/TextDetailsFormatter.php @@ -0,0 +1,118 @@ + // Use the field label as a fallback. ... + // Use the field label as a fallback.
Can be removed when the comment is changed.
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ +use Drupal\field\Entity\FieldConfig;
Sort the use statements.
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ + * Tests the text formatters functionality.
As I read this, this tests is testing multiple text formatters. Let's update the comment to reflect that this is testing a specific text formatter.
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ +class TextDetailsFormatterTest extends EntityKernelTestBase {
This test has the same setup as TextFormatterTest, does it make sense to extend from that?
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ + /** + * Modules to enable. + * + * @var array + */
This can be inheritdoc.
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ + * Sets of arguments to pass to the formatter.
Let's be more descriptive. Use an item list to describe the array values.
-
+++ b/core/modules/text/tests/src/Kernel/TextDetailsFormatterTest.php @@ -0,0 +1,147 @@ + return [
Why is there no case with 'open' set to TRUE?
-
+++ b/core/modules/text/text.module @@ -32,6 +32,8 @@ function text_help($route_name, RouteMatchInterface $route_match) { + $output .= '<dt>' . t('Displaying the text inside a details group.') . '</dt>';
This is a heading and does not need a full stop.
Finally, I think that this new formatter should be added to the list being tested in \Drupal\Tests\text\Kernel\TextFormatterTest.
I want to take another look at TextDetailsFormatterTest.
-
- 🇮🇹Italy kopeboy Milan
I would also consider allowing the end user to input the summary text, and even supporting the existing "Text with summary" field type when the summary is required (in this case, check current bug at 🐛 After enabling Require Summary on a field can't save the field Needs review ).
- 🇮🇹Italy kopeboy Milan
We can take inspiration from the details_summary_field_formatter → module.