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.
CommentFileSizeAuthor
#13 interdiff_10_13.txt545 bytesanmolgoyal74
#13 3176910-13.patch1.76 KBanmolgoyal74
#10 interdiff_6-10.txt514 byteskishor_kolekar
#10 3176910-10.patch1.79 KBkishor_kolekar
#6 3176910-6.patch1.78 KBkostyashupenko
#4 3176910-4.patch855 byteshansa11

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
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

hansa11 commented
Status: Active » Needs review
StatusFileSize
new 3176910-4.patch855 bytes

Please review.

Thanks!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
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

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
StatusFileSize
new 3176910-6.patch1.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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented 
+++ 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

kishor_kolekar commented
Assigned: Unassigned » kishor_kolekar
kishor_kolekar’s picture

kishor_kolekar commented
Assigned: kishor_kolekar » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3176910-10.patch1.79 KB
new interdiff_6-10.txt514 bytes

Worked on comment #8

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Reviewed & tested by the community

#11 looks great!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
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

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3176910-13.patch1.76 KB
new interdiff_10_13.txt545 bytes
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
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

lauriii
he/him
Primary language Finnish
Location Finland
commented
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.