Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OEL-1618: Upgrade BCL to 0.23.0. #144

Merged
merged 15 commits into from
Jun 24, 2022
Merged

OEL-1618: Upgrade BCL to 0.23.0. #144

merged 15 commits into from
Jun 24, 2022

Conversation

escuriola
Copy link
Contributor

No description provided.

@donquixote
Copy link
Contributor

This seems to be ok.
However, I suspect we now have duplicate <article> tags in node (teaser) templates.
(I think only event teaser and news teaser)

Let's:

  • Remove the outer <article> wrapper, if we include a card pattern that already does add an article wrapper via bcl-card.
  • Pass the node attributes into the pattern.
  • Add a test line to count the article tags when we render a news or event teaser.

Also, after changing card h* tags in oebt to h1, we need to update the tests here in whitelabel.

@escuriola escuriola force-pushed the OEL-1618 branch 2 times, most recently from 879d816 to f653d87 Compare June 1, 2022 18:32
@donquixote
Copy link
Contributor

I notice we have a bunch of templates where we put custom hx elements with fw-bold class.
I found these by looking for "fw-bold" in templates, and then checking if it is used for hx elements.

templates/content/field--node--body--oe-sc-event.html.twig
templates/content/field--node--oe-documents--oe-sc-event.html.twig
templates/overrides/navigation/oe-corporate-blocks-eu-footer.html.twig
templates/overrides/navigation/oe-corporate-blocks-neutral-footer.html.twig
templates/paragraphs/paragraph--oe-description-list.html.twig
templates/paragraphs/paragraph--oe-rich-text.html.twig

We need to compare these to netlify. In most cases I checked, the hx tags in netlify don't have the fw-bold class.

We could also consider, instead of having custom title elements, to use the new option of sending the title as a parameter to the pattern or bcl template. Unfortunately the patterns currently do not support this option.

TBD if we do this here or in a follow-up issue.

@donquixote
Copy link
Contributor

Conclusion: Fix these in the same issue.

@escuriola escuriola force-pushed the OEL-1618 branch 2 times, most recently from aaeec65 to 775b6ab Compare June 3, 2022 11:49
@donquixote
Copy link
Contributor

I listed 6 templates above. I see only 2 of them were changed.

@escuriola escuriola force-pushed the OEL-1618 branch 3 times, most recently from 84a347e to 639905c Compare June 10, 2022 07:34
@donquixote
Copy link
Contributor

@escuriola And now project has the wrong heading fonts!
templates/content/node--oe-project--full.html.twig
Sorry!

templates/content/field--node--body--oe-sc-event.html.twig Outdated Show resolved Hide resolved
content.oe_sc_event_dates|field_value,
],
attributes: attributes,
}) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to 2nd reviewer:

  • The article tag is now included in card pattern.
  • Passing 'attributes' to the card pattern means that all the entity attributes will now be on the card.

@donquixote
Copy link
Contributor

@escuriola And now project has the wrong heading fonts! templates/content/node--oe-project--full.html.twig Sorry!

And list page too - netlify has h2 for heading on top of the results.
templates/list_pages/node--oe-list-page--full.html.twig

also search page - netlify has h2 for heading on top of the results.
templates/overrides/search/block--facets-summary-block.html.twig

Perhaps there is more.

templates/content/field--node--body--oe-sc-event.html.twig Outdated Show resolved Hide resolved
{%- endset -%}
{% include '@oe-bcl/bcl-heading/heading.html.twig' with {
title: _label,
title_tag: 'h2',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h2 is the default, we can omit it.
Not a blocker though.

templates/content/node--oe-project--full.html.twig Outdated Show resolved Hide resolved
templates/list_pages/node--oe-list-page--full.html.twig Outdated Show resolved Hide resolved
templates/list_pages/node--oe-list-page--full.html.twig Outdated Show resolved Hide resolved
@escuriola escuriola force-pushed the OEL-1618 branch 2 times, most recently from 72883c8 to e298c73 Compare June 16, 2022 15:09
donquixote
donquixote previously approved these changes Jun 16, 2022
@@ -33,7 +33,7 @@ protected function getAssertions($variant): array {
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brummbar brummbar merged commit f3b5dab into 1.x Jun 24, 2022
@brummbar brummbar deleted the OEL-1618 branch June 24, 2022 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants