From @lauriii in #216 in #3111409: Add new Olivero frontend theme to Drupal 9.1 core as beta

+++ b/core/themes/olivero/templates/includes/preload.twig
@@ -0,0 +1,7 @@
+{#
+  Preload the fonts for the headings and normal body copy (non bold and non italic).
+ #}
 
This should be converted to @file documentation and should include variables passed for the template.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

mherchel’s picture

Title: [Code Review] move preload.twig documentation to @file and include variables passed for the template. » [Olivero Code Review] move preload.twig documentation to @file and include variables passed for the template.
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 9.1.x-dev
Component: Code » Olivero theme
Priority: Normal » Minor
mherchel’s picture

Title: [Olivero Code Review] move preload.twig documentation to @file and include variables passed for the template. » Move Olivero's preload.twig documentation to @file and include variables passed for the template.
hansa11’s picture

Status: Active » Needs review
FileSize
855 bytes

Please review.

Thanks!

mherchel’s picture

Status: Needs review » Needs work

This file will also contain all the variables passed down from html.html.twig.

We could use the only keyword (see https://twig.symfony.com/doc/2.x/tags/include.html) to only pass the olivero_path variable.

Also, while you're modifying this, can you add the olivero_path variable to html.html.twig? It's missing from there.

kostyashupenko’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

By the way, why we don't declare woff fonts on preload, but only woff2 ? Wondering if issues in IE browser

Status: Needs review » Needs work

The last submitted patch, 6: 3176910-6.patch, failed testing. View results

mherchel’s picture

+++ b/core/themes/olivero/templates/includes/preload.twig
@@ -1,6 +1,12 @@
+ * - olivero_path: Returns the path to an Olivero theme

This should say "Returns the path to the Olivero theme." (replace "an" with "the", and add a period)

kishor_kolekar’s picture

Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

Assigned: kishor_kolekar » Unassigned
Status: Needs work » Needs review
FileSize
1.79 KB
514 bytes

Worked on comment #8

mherchel’s picture

Status: Needs review » Reviewed & tested by the community

#11 looks great!

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/themes/olivero/templates/includes/preload.twig
@@ -1,6 +1,12 @@
+ * Olivero's implementation to preload the non-bold and non-italic fonts for the headings and the body copy.

According to our coding standards, the file summary should be one line of up to 80 characters ending in "."

anmolgoyal74’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
545 bytes
mherchel’s picture

Status: Needs review » Reviewed & tested by the community

#13 looks perfect!

  • lauriii committed 2108025 on 9.2.x
    Issue #3176910 by kishor_kolekar, anmolgoyal74, hansa11, kostyashupenko...

  • lauriii committed 6083de9 on 9.1.x
    Issue #3176910 by kishor_kolekar, anmolgoyal74, hansa11, kostyashupenko...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2108025 and pushed to 9.2.x and 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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