OEL-1159: Align search variant with default card pattern #178

donquixote · 2022-02-09T20:35:16Z

Jira issue(s):

OEL-1159

escuriola · 2022-03-17T17:11:58Z

templates/patterns/card/card.ui_patterns.yml

@@ -10,47 +10,81 @@ card:
      description: 'Card with special behaviour for print search results items.'
  fields:
    orientation:
+      # Full type: `'horizontal'|'vertical'`.


We don't have any comment. Please remove them.

escuriola · 2022-03-17T18:39:01Z

templates/patterns/card/card.ui_patterns.yml

      preview: 'Title card'
+    url:


I'm not agree with the url field. We already have the title field. I don't feel comfortable with two fields for the same when we are trying to keep the patterns as simple as possible.

Also If anybody needs a link then they maybe want to customise, a custom class, title, etc. And with url you loose this possibility you have with the title field. Maybe if instead url it was a link pattern and then print the link in the template, it would have more sense. But IMHO with title field is enough for cover this.

I am ok to see it removed.
The motivation was that calling code should not have to set the 'underline-hover' class on the link.
But ok for me to remove.

escuriola · 2022-03-18T08:26:31Z

templates/patterns/card/pattern-card--variant-search.html.twig

+ *
+ * Differences to main variant:
+ *   - Always use horizontal layout, with specific classes.
+ *   - Add extra classes on card body (right column), to prevent padding.
+ *     This should probably be handled by BCL if horizontal is used.
+ *   - Use text color dark.
+ *   - Set different margin-bottom for text element.
+ *   - Set special parameters on the image.
+ *   - Add special wrapper attributes.
+ *   - Ignore 'subtitle', 'date', 'card_header', 'card_footer'.


Really need this? Please remove.

escuriola · 2022-03-18T08:33:09Z

templates/patterns/card/card.ui_patterns.yml

      type: text
      label: 'Card footer'
      description: 'Card footer content.'
      preview: 'Footer of card'
    badges:
+      # Full type: `list<array{label: string|render|markup, ..}>`.
+      # See 'bcl-badge.html.twig'.
      type: array
      label: 'Badges'
      description: 'List of badges for the card.'


The badges label are primary and secondary but has the same aspect, so I would change the label or add the badge style to the badges.

I am confused by the wording of this comment.

I think this is good now.

escuriola · 2022-03-18T08:36:26Z

templates/patterns/card/pattern-card.html.twig

-  'subtitle': subtitle,
-  'text': text,
+    'tag': 'div',
+  } : {},
  'content': content,
  'image': image,
  'card_header': header,
  'card_footer': footer,
  'badges': badges,


default badges has set the outline to true.

escuriola · 2022-03-18T08:39:46Z

templates/patterns/card/pattern-card.html.twig

+  'date': (date is not empty) ? {
+    'day': date|date('d'),
+    'month': date|date('M'),
+    'year': date|date('Y'),
+    'date_time': date|date('Y-m-d'),
+  } : {},


We can remove date until full date ticket is prepared.

I notice that date is not even declared in the pattern definition.
I kept it there because it existed in the current version of the template.
But I agree to remove it.

escuriola · 2022-03-21T08:40:38Z

templates/patterns/card/card.ui_patterns.yml

@@ -35,6 +35,15 @@ card:
      preview:
        path: 'https://placeimg.com/1000/500/people'
        alt: 'Alternative text for card image'
+    meta:
+      # Full type: `list<text|render|markup>`.


Please remove comments

brummbar

Some minor remarks.

tests/src/Kernel/fixtures/markup_rendering_patterns/card.yml

brummbar · 2022-03-25T21:16:47Z

templates/patterns/card/pattern-card--variant-search.html.twig

@@ -5,58 +5,54 @@
 */
 #}
 {% spaceless %}


Let's take the chance to replace these with {% apply spaceless %} as this one used above is deprecated already.

brummbar · 2022-03-25T21:21:35Z

templates/patterns/card/pattern-card--variant-search.html.twig

+  {% endfor %}
+{% endif %}
+{% include '@oe-bcl/card' with {
+  horizontal: true,


In the other template we quote the keys, in here we don't. We should stick to one, I believe we went for always quote.

donquixote · 2022-04-04T11:23:03Z

This will break whitelabel and showcase.
Let's not merge before the demo!

…nts.

…cl-card.

… variant.

Also add trailing comma after last array value.

Any <p> tag should be sent with the content of the 'text' parameter.

…tern.

This reverts commit 0034c57.

This reverts commit 8c07b90.

…twig. This reverts commit e0d892a.

The parameter was not declared in the pattern definition. A new date parameter might be introduced in the future.

Changes: - Make 'rounded_pill: true, outline: true' the default for all badges in cards. This is the most common way badges are shown in cards. - Preserve custom settings when merging defaults. - Expose the full signature of bcl-badge in the badge parameter. (this was previously not the case for search variant) - Have 3 badges in the preview, to show different options.

…ion.

drishu · 2022-04-06T12:18:00Z

templates/patterns/card/pattern-card--variant-search.html.twig

-    content: content
-  } only %}
-{% endspaceless %}
+  <div>{{ content }}</div>


is the compulsory div intentional ?

drishu · 2022-04-06T12:28:43Z

templates/patterns/card/pattern-card.html.twig

+      {% endfor %}
+    </div>
+  {% endif %}
+  <div>{{ content }}</div>


is the compulsory div intentional ?

I think @brummbar mentioned we should wrap this in a div. I don't have a strong opinion on it myself.
In earlier versions of the theme, the idea was that "content" is like a general-purpose variable where developers can put additional stuff that is not already handled with other variables. so it would support more than one element, and these elements would already have their own wrappers.

This said: If we do keep the div, we need to check first that the content is not empty.

So we can either drop the div, or check emptiness first.

drishu · 2022-04-06T12:31:05Z

templates/patterns/card/pattern-card--variant-search.html.twig

+  'text': (text is not empty) ? {
+    'content': text,
+    'classes': 'mb-2-5',
+    'tag': 'div',


why div ? it was p before, I see p in netlify..

We want to support formatted text which may already contain <p> tags.
Then the <div> is just an additional wrapper.

drishu · 2022-04-06T12:40:18Z

templates/patterns/card/pattern-card--variant-search.html.twig

+{% apply spaceless %}
+{% set content %}
+  {% if meta is not empty %}
+    {# Use negative margin to compensate for me-3 on each item. #}


lets remember to revisit this with BCL 0.21

ok but not now, right?

It turns out that 'content is empty' is not sufficient, because e.g. ['#markup' => ''] won't be determined as empty. Also, 'content|render' does not behave as desired, if content is already a Markup object.

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch 2 times, most recently from a08173e to ab81793 Compare February 9, 2022 21:48

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from ab81793 to 0034c57 Compare March 17, 2022 17:09

escuriola suggested changes Mar 18, 2022

View reviewed changes

escuriola suggested changes Mar 21, 2022

View reviewed changes

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from e892485 to 97dfdfd Compare March 21, 2022 12:56

escuriola previously approved these changes Mar 21, 2022

View reviewed changes

brummbar requested changes Mar 25, 2022

View reviewed changes

donquixote dismissed escuriola’s stale review via e873d97 March 30, 2022 11:15

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from e873d97 to 63fea3a Compare March 30, 2022 11:16

brummbar previously approved these changes Apr 1, 2022

View reviewed changes

escuriola previously approved these changes Apr 4, 2022

View reviewed changes

escuriola dismissed stale reviews from brummbar and themself via dd75b38 April 5, 2022 10:55

escuriola force-pushed the OEL-1159-card-pattern-search-variant branch from 63fea3a to dd75b38 Compare April 5, 2022 10:55

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from c4db4bb to 453ff3c Compare April 5, 2022 19:31

donquixote added 13 commits April 6, 2022 14:25

OEL-1159: Document that 'title' also supports markup and render eleme…

023876e

…nts.

OEL-1159: Align 'image' parameter type with default variant.

d49b9e0

OEL-1159: No longer add 'me-2' class to badges, this is now done in b…

4e32c1a

…cl-card.

OEL-1159: Remove bogus keys for badges in search variant test.

27e59d1

OEL-1159: Don't support render element structure for badges in search…

27967e3

… variant.

OEL-1159: Use inline ternary expressions instead of local vars.

04340ad

Also add trailing comma after last array value.

OEL-1159: No more special handling of text parameter in search variant.

c797e89

Any <p> tag should be sent with the content of the 'text' parameter.

OEL-1159: Document how search variant is different from base card pat…

b5e4af3

…tern.

OEL-1159: Add comments in card pattern definition.

15f7b11

OEL-1159: CS: Comma after last array value.

6e44953

OEL-1159: CS: No extra indentation level for spaceless.

057821f

OEL-1159: OPTIONAL: Support 'meta' in all card variants.

7d459b6

OEL-1159: OPTIONAL: Support 'url' in all card variants.

1c1b1fe

donquixote added 9 commits April 6, 2022 14:25

OEL-1149: Revert adding the 'url' parameter for the pattern.

dee1b8f

This reverts commit 0034c57.

OEL-1159: Revert adding comments in card pattern definition.

d6313bf

This reverts commit 8c07b90.

OEL-1159: Revert adding comment in pattern-card--variant-search.html.…

e65837f

…twig. This reverts commit e0d892a.

OEL-1159: Remove 'date' parameter in pattern-card.html.twig.

d947afc

The parameter was not declared in the pattern definition. A new date parameter might be introduced in the future.

OEL-1159: Remove comment for 'meta' parameter in card pattern definit…

d49fb4b

…ion.

OEL-1159: Use apply spaceless in twig templates.

b772fba

OEL-1159: Quote keys in twig templates.

e769f77

OEL-1159: Verify number of meta items in fixture.

4870149

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from 453ff3c to 4870149 Compare April 6, 2022 12:25

drishu requested changes Apr 6, 2022

View reviewed changes

donquixote added 3 commits April 6, 2022 15:41

OEL-1159: Rename local variable _content in template.

be1e565

OEL-1159: Check if content is empty, the simple way.

cf0dbe4

OEL-1159: Check if content is empty, the safe way.

1dc402a

It turns out that 'content is empty' is not sufficient, because e.g. ['#markup' => ''] won't be determined as empty. Also, 'content|render' does not behave as desired, if content is already a Markup object.

donquixote force-pushed the OEL-1159-card-pattern-search-variant branch from 9f84037 to 1dc402a Compare April 6, 2022 14:20

OEL-1159: Also check emptiness of content in default variant.

34dbd45

drishu approved these changes Apr 6, 2022

View reviewed changes

escuriola approved these changes Apr 6, 2022

View reviewed changes

donquixote merged commit d66d1f2 into 1.x Apr 6, 2022

donquixote deleted the OEL-1159-card-pattern-search-variant branch April 6, 2022 15:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OEL-1159: Align search variant with default card pattern #178

OEL-1159: Align search variant with default card pattern #178

donquixote commented Feb 9, 2022

escuriola Mar 17, 2022

escuriola Mar 17, 2022

donquixote Mar 18, 2022

escuriola Mar 18, 2022

escuriola Mar 18, 2022 •

edited

Loading

donquixote Mar 18, 2022

donquixote Apr 6, 2022

escuriola Mar 18, 2022

escuriola Mar 18, 2022

donquixote Mar 18, 2022

escuriola Mar 21, 2022

brummbar left a comment

brummbar Mar 25, 2022

brummbar Mar 25, 2022

donquixote commented Apr 4, 2022

drishu Apr 6, 2022

donquixote Apr 6, 2022

drishu Apr 6, 2022

donquixote Apr 6, 2022

drishu Apr 6, 2022

donquixote Apr 6, 2022 •

edited

Loading

drishu Apr 6, 2022

donquixote Apr 6, 2022

OEL-1159: Align search variant with default card pattern #178

OEL-1159: Align search variant with default card pattern #178

Conversation

donquixote commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escuriola Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brummbar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donquixote Apr 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

escuriola Mar 18, 2022 •

edited

Loading

donquixote Apr 6, 2022 •

edited

Loading