Add new Olivero frontend theme to Drupal 9.1 core as beta

Excited about this! Bartik had its place, but this is long overdue.

https://www.lullabot.com/articles/update-status-drupals-new-olivero-theme says:

Olivero was initially slated for inclusion in core in Drupal 9.1. That’s still the most likely scenario. That said, there’s a possibility that Drupal may shift the 9.0 beta deadline to the end of April. If that’s the case, there is a possibility to submit a core patch beforehand.

To commit by this time, we need to submit the patch a minimum of a few weeks ahead of time to give core committers time to review (and even that might not be enough time).

We’re currently working on [META] Add new default Olivero frontend theme to Drupal 9 core to define the minimum beta requirements to submit to core. Expect this issue to be more fleshed out within the coming days.

Note that a few weeks is still very tight lead time for a core review of a new theme. #3079738: Add Claro administration theme to core took six weeks from a complete patch to alpha stability, and then it was another week or two before it was marked beta. And that was when the frontend framework manager had already been working on and reviewing the theme for months before that, so was familiar with everything. So the timeline is very tight for core reviewers even if we end up delaying beta1 until May.

That said, having a complete roadmap here soon and an initial prototype posted within the next two months for an alpha experimental experimental theme (that would not be released in 9.0.0, but committed to the development branch) would still significantly increase the chances that the theme could be released as a stable theme in 9.1, because we'd have a full six months of development time from the first core prototype until the 9.1 beta deadline.

A couple changes we'll need right off the bat for Drupal 9 compatibility:

1. We are currently upgrading from Normalize.css major version 3 all the way to 8, which has required numerous changes to core themes and caused some regressions.

   See here: #2821525: Update normalize.css to the most recent version

   This will likely impact Olivero as well.
    

2. Core themes are also being decoupled from Classy and Stable (so that they have no base theme and receive markup/accessibility/styling bugfixes directly in each issue for each minor release). See:

   • #2659890: [Policy] [Plan] Drupal 9 and 10 markup and CSS backwards compatibility
   • #3050389: [META] Remove dependency to Classy from core themes
   • #3110855: Plan for removing dependency to Stable in Bartik/Seven/Claro/Umami

   Olivero will need the same changes.

@xjm Thank you for letting us know about the Drupal 9 theme compatibility. We will make sure that the Olivero theme does not have any dependency on the Classy theme.

#3083081: Additional accessibility testing on design of new theme is poorly scoped. Can I have some feedback on the comments I left there?

9.0.0-beta1 has been released, so moving this to the 9.1.x branch as I'm triaging our beta should-haves. 🙌

Holy cow! It's the first Olivero core patch!! 😍 🥰 😘 😅 😂 🤣 🤪 😇 🤯 🙌

Note this just adds it to core, it doesn't set the default theme from Bartik to this.

Minor title tweak; version has it's own field.

I don't understand #35 - There is a version field, and it clearly says 9.1.x-dev, not 8.

There's nothing special about 9.1. I'm very concerned about how much of the accessibility maintainers's time is taken up chasing the arbitrary urgency of strategic initiatives, at the expense of fixing accessibility problems in modules which are already marked stable, or completing work which earlier initiatives left for follow-up (so they could hit their arbitrary version target). In particular, I'm thinking of #3002770: Provide authors with tools to manage transcripts and captions/subtitles for local video and audio which is a large WCAG hole created by the media initiative not using the accessibility gate. I don't want to treat Olivero or Claro as more urgent than that, and leave the WCAG time-based media criteria until 9.2 or 9.3 (because that would make us as bad as Twitter).

Moved all of the PostCSS dependency issues to the “stable” criteria.

The title is a bit confusing -- there is no way Olivero will be the new default frontend theme of 9.1. The minimum goal is to add it to core as an alpha experimental theme, although the team might want to target beta or stable as described above. However, even if it gets to stable (including passing the accessibility gate), we would not want to switch the default theme of Standard until Olivero had already been stable in core and had any standard profile blockers solved.

I do share @andrewmacpherson's concern about should-have accessibility work not being completed when features are marked stable. (#3002770: Provide authors with tools to manage transcripts and captions/subtitles for local video and audio is an example but also see all the outstanding issues on #2834729: [META] Roadmap to stabilize Media Library, #3007978: Accessibility Plan for Layout Builder, etc.) The accessibility aspect of this is part of a larger need to finish the work we've started on these features, and for many of them, outstanding accessibility and UX work are part of what's blocking making them part of the Standard profile. I believe the DrupalCon Global keynote will include some thoughts about that aspect of the features in core so far.

It's worth noting though that as @Gábor Hojtsy points out, the same people who might implement media captioning are not the ones working on this theme. On the other hand, setting per-minor goals does create pressure on maintainers to provide reviews in time for those goals, so that is something for any initiative team to be conscientious of.

Updating the preview link as per #3157745-2: "Latest Tugboat Preview" link broken

Is there a more recent version patch for Olivero than #17 or should I start by testing the contrib project? Also, is the roadmap written currently so that Olivero will be included in core as stable?

#17 is the latest, although there will be a new patch shortly (I have one more issue that I want to tackle, and then roll a release)

It won't be included as stable... I would love that to happen, but I don't have the time to make it happen in that short period of time.

It would probably make sense to create a new milestone before beta for the core inclusion must haves. This would be helpful because Olivero could be added to core code base, even if it's not beta. The PostCSS related issues should be moved into the group that lists must have issues for inclusion since when Olivero is in core, it should rely on the same PostCSS tooling that we use for Claro.

Updated core patch attached. Will address the last couple comments in a bit.

1. What is the difference between css/dist and css/src folders? Both of them seem to contain both, .pcss.css and .css files.
2. Let's make that PHP, JavaScript and CSS pass our code style tests, as well as spell checks
3. The patch has some JavaScript that is touching markup and not using behaviors. We should update those to use behaviors to make sure they are compatible with the ajax system. It would be also great if we could get a JavaScript subsystem maintainer to review the patch.
4. We should add some basic test coverage for Olivero to make sure that most important features work as expected
5. Let's make sure all SVG files have been optimized. This could be done with ImageOptim for example.
6. Some key features are not functional for no-JS users. We should make sure that for example, menu and search work even without JavaScript.
7. 
   Let's make sure settings tray works as expected with Olivero. At the moment some styles are leaking to Settings Tray.
8. 
   Could we handle the case of having too many menu items more gracefully?

9. +++ b/core/themes/olivero/templates/content/node.html.twig
   @@ -0,0 +1,113 @@
   +    {{ content|without('comment') }}
   ...
   +      {{ content.comment }}
   

   Is there a specific reason to disallow configuring the location of comments in the Field UI?

10. +++ b/core/themes/olivero/templates/misc/status-messages.html.twig
    @@ -0,0 +1,68 @@
    + * Theme override for status messages.
    

    We should override Drupal.theme.message too to make sure JS messages get rendered correctly.

11. +++ b/core/.stylelintignore
    @@ -1,2 +1,4 @@
    diff --git a/core/.prettierrc.json b/core/themes/olivero/.prettierrc.json
    
    diff --git a/core/.prettierrc.json b/core/themes/olivero/.prettierrc.json
    similarity index 100%
    
    similarity index 100%
    copy from core/.prettierrc.json
    
    copy from core/.prettierrc.json
    copy to core/themes/olivero/.prettierrc.json
    

    Is this intentional?

12. +++ b/core/themes/olivero/js/navigation.js
    --- /dev/null
    +++ b/core/themes/olivero/js/polyfills.es6.js
    

    We should document the source and the license of the polyfills here

13. +++ b/core/themes/olivero/js/polyfills.es6.js
    @@ -0,0 +1,16 @@
    +if (window.NodeList && !NodeList.prototype.forEach) {
    

    Let's add todo to remove this once #3143465: Add NodeList.forEach polyfill to support IE11 has landed.

14. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +  // olivero has custom styling for the maintenance page.
    

    Nit: s/olivero/Olivero

15. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +  // Remove the "Add new comment" link on teasers or when the comment form is
    +  // displayed on the page.
    +  if ($variables['teaser'] || !empty($variables['content']['comments']['comment_form'])) {
    +    unset($variables['content']['links']['comment']['#links']['comment-add']);
    

    Let's open an issue to allow configuring this.

16. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +  // Apply custom date formatter to "date" field.
    +  if (!empty($variables['date']) && !empty($variables['display_submitted']) && $variables['display_submitted'] === TRUE) {
    +    $variables['date'] = \Drupal::service('date.formatter')->format($variables['node']->getCreatedTime(), 'custom', 'j  F,  Y');
    +  }
    @@ -0,0 +1,576 @@
    +   $variables['info_date'] = \Drupal::service('date.formatter')->format($variables['result']['node']->getCreatedTime(), 'custom', 'j  F,  Y');
    

    Shouldn't this be controllable by the date and time formats settings? I think it's important because the format could vary depending on the language or region.

17. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +      $suggestions[] = 'block__' . $region . '__' . $variables['elements']['#plugin_id'];
    +      $suggestions[] = 'block__' . $region . '__' . $variables['elements']['#id'];
    

    Let's prefix the final section of the suggestion to avoid overlaps

18. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +    $variables['table']['#header'][0]['data'] = [
    +      '#type' => 'html_tag',
    +      '#tag' => 'h4',
    +      '#value' => $variables['element']['#title'],
    +      '#attributes' => $header_attributes,
    +    ];
    

    Let's add a todo to change this after #3099026: Claro's preprocessing of field multiple value form's table header cell removes potential changes by others has landed

19. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,576 @@
    +  $variables['created'] = \Drupal::service('date.formatter')->formatInterval(REQUEST_TIME - $date) . ' ago';
    

    This should be translatable

20. +++ b/core/themes/olivero/templates/layout/page.html.twig
    @@ -0,0 +1,141 @@
    +        {# TODO: Add RSS Social Block region #}
    

    Should we remove this for now since I assume this isn't needed until the social block region is added?

Phew! First block of replies below. Will work on others shortly.

1. What is the difference between css/dist and css/src folders? Both of them seem to contain both, .pcss.css and .css files.

This is a bug in our core patch script. Issue filed at #3165448: Patch script generates *.pcss.css fies within the dist directory

2. Let's make that PHP, JavaScript and CSS pass our code style tests, as well as spell checks

We have a meta issue at #3124796: META: Adjust Olivero codebase to meet Drupal coding standards.

3. The patch has some JavaScript that is touching markup and not using behaviors. We should update those to use behaviors to make sure they are compatible with the ajax system. It would be also great if we could get a JavaScript subsystem maintainer to review the patch.

Note that some of the markup is inserted through the page.html.twig, and should not need behaviors (let me know if this is incorrect). We're using behaviors in menus and messages. I will do some additional inventorying to check to see if there's anything else.

4. We should add some basic test coverage for Olivero to make sure that most important features work as expected

We have an issue for this at #3135511: Create basic test coverage for Olivero theme.

5. Let's make sure all SVG files have been optimized. This could be done with ImageOptim for example.

Opened #3165449: Ensure all SVGs are optimized

6. Some key features are not functional for no-JS users. We should make sure that
for example, menu and search work even without JavaScript.

The wide viewport of the navigation and search currently work without JS. The mobile menu currently does not (opened #3165450: Ensure mobile menu is displayed when JS is disabled)

7. Let's make sure settings tray works as expected with Olivero. At the moment some styles are leaking to Settings Tray.

#3149714: Olivero: Audit form items within settings tray for visual inconsistencies

8. Could we handle the case of having too many menu items more gracefully?

We're looking into it. Currently you can go into the theme settings and change to an "always on" mobile type navigation. I have some reservations about handing this automatically. We can discuss at some point.

9. Is there a specific reason to disallow configuring the location of comments in the Field UI?

Not sure. This might be something we copied over from another theme. Opened #3165451: Olivero should allow the placement of comments via field ui

10. We should override Drupal.theme.message too to make sure JS messages get
rendered correctly.

Created #3165452: Override Drupal.theme.message to to make sure JS messages get rendered correctly

11. Is this [.prettierrc.json] intentional?

Nope. Looks like an issue with patch genereation script. Opened #3165453: Patch generation script unintentionally copying core's .prettierrc.json

12. We should document the source and the license of the polyfills here

Opened #3165454: Document source and license of polyfills

13. Let's add todo to remove this once #3143465: Add NodeList.forEach polyfill to
support IE11 has landed.

Opened #3165455: Add @todo to remove nodeList.forEach() when core patch lands.

14. Nit: s/olivero/Olivero

This is weird. When I look at the code, it is capitalized correctly. See https://git.drupalcode.org/project/olivero/-/blob/8.x-1.x/olivero.theme#L79

15. Let's open an issue to allow configuring this [removing the comments link from the teaser].

Should this be a core issue? Or a setting in Olivero?

16. Shouldn't this [date format in teaser] be controllable by the date and time formats settings? I think it's important because the format could vary depending on the language or region.

That makes a lot of sense. Opened issue #3165970: Teaser date/time format should not be hardcoded

17. Let's prefix the final section of the suggestion to avoid overlaps

Opened #3165971: Prefix final section of block template suggestions in olivero.theme.

Let's add a todo to change this after #3099026: Claro's preprocessing of field multiple value form's table header cell removes potential changes by others has landed

Opened #3165972: Add todo on form table header code block in theme file

This [text string 'ago'] should be translatable

Opened #3165973: Text string "ago" should be translatable within theme file

Should we remove this [@todo] for now since I assume this isn't needed until the social block region is added?

Opened #3165975: Remove unneeded todo

@mherchel and I reviewed all of the issues in the Beta and Stable criterias and we adjusted the roadmap to reflect the remaining work that's needed for the theme.

Updated patches for the latest tag (beta2) of Olivero attached!

Reviewed the list of blockers with @mherchel, @proeung, and @Gábor Hojtsy. Made a bunch of recommendations in terms of de-scoping from "must-haves". There was very good work that was done to make sure that the technical debt was adequately captured here, which is important for release management review. However, a lot of things in the list qualify as "yeah we ought to fix that" or "actually that's more of a support request" and so on. We banged through the list to cut it down tremendously so it's mainly focused on the gates (attention to docs, CSS interoperability, accessibility, etc.). (@mherchel is going to be updating the issue summary to reflect this.)

I reviewed the demo site and clicked on lots of things, uncovering new delight about every ~7.2 seconds. The logo-to-hamburger menu! The search box! The responsive grids! The attention to detail here is superb, and as much as I tried to break something, I couldn't. Well done, everyone. I would love to see us view "must-haves" through a very critical lens, so we can get this in front of users as soon as possible because this is such a tremendous step forward! :O GREAT WORK, Olivero team!!

Got my comment eaten, writing the short version, sorry.

1. Olivero will be able to use the vanilla once script once it's in core, for now it's ok to leave it like that. It doesn't make sense to require jquery for such a small thing. the once feature is handled Drupal 6 style. good enough for now.
2. There are missing parameters in a few closures (scripts, navigation, second-level-navigation):
   

   (Drupal => {
     // code
   }))(Drupal);

3. in scripts the drupalSettings object is used to pass around functions between scripts. drupalSettings is only for json objects going from the backend to the frontend. If something like this is needed it should live in the Drupal object, not drupalSettings. On the same note, it is not the job of scripts to declare the drupalSettings object (in scripts.es6.js) here it was needed because of a missing core/drupalSettings dependency in the libraries file.
4. There is a props.body.classList.remove('js-overlay-active', 'js-fixed') that should be 2 lines for IE11 compat. (only one argument supported for the remove function).

In any case really glad to see some vanilla JS. Once the drupalSettings thing is cleared up it's good to go on the JS side.

I just update our rodmap based on the recommendations from the product managers, @webchick and @Gábor Hojtsy. All of the issues should be placed in the correct criterias.

New patch! I'm also including the interdiff since the last review (comment 47).

I reviewed most of the CSS. Here's what I could find so far:

1. +++ b/core/themes/olivero/css/components/comments.pcss.css
   @@ -0,0 +1,245 @@
   +.comments {
   +  > .comment {
   ...
   +    ~ .comment {
   

   Could we provide a CSS class indicating whether the comment is on the main level or not instead of using CSS? Technically this is against BEM since block elements shouldn't depend on other elements on the page.

2. +++ b/core/themes/olivero/css/components/comments.pcss.css
   @@ -0,0 +1,245 @@
   +  .text-content {
   ...
   +  .links {
   

   Can we customize the markup so that either .links and .text-content become their own blocks or they are rendered as elements inside .comment?

3. +++ b/core/themes/olivero/css/components/field-image.pcss.css
   @@ -0,0 +1,51 @@
   +.page-node-type-article {
   +  .field--name-field-image {
   

   Could we make this a reusable component so that it's not tied to the article node type and field with name image?

4. +++ b/core/themes/olivero/css/components/fieldset.pcss.css
   @@ -0,0 +1,136 @@
   +.fieldset__legend {
   ...
   +  .fieldset__label {
   +    &.form-required {
   +      &:after {
   +        background-image: url("data:image/svg+xml,%3Csvg height='16' width='16' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='m0 7.562 1.114-3.438c2.565.906 4.43 1.688 5.59 2.35-.306-2.921-.467-4.93-.484-6.027h3.511c-.05 1.597-.234 3.6-.558 6.003 1.664-.838 3.566-1.613 5.714-2.325l1.113 3.437c-2.05.678-4.06 1.131-6.028 1.356.984.856 2.372 2.381 4.166 4.575l-2.906 2.059c-.935-1.274-2.041-3.009-3.316-5.206-1.194 2.275-2.244 4.013-3.147 5.206l-2.856-2.059c1.872-2.307 3.211-3.832 4.017-4.575-2.081-.402-4.058-.856-5.93-1.356' fill='%23ffffff'/%3E%3C/svg%3E%0A");
   +      }
   +    }
   

   How is this dependent on .fieldset__legend? Couldn't this be defined with the other .fieldset__label styles?

5. +++ b/core/themes/olivero/css/components/fieldset.pcss.css
   @@ -0,0 +1,136 @@
   +.fieldset__legend--visible ~ .fieldset__wrapper {
   ...
   +  .fieldset--group & {
   

   Where this can be tested?

6. +++ b/core/themes/olivero/css/components/footer.pcss.css
   @@ -0,0 +1,42 @@
   +.site-footer {
   ...
   +  .menu {
   

   Should we create new component for the footer menu instead of allowing placing any menu in the footer?

7. +++ b/core/themes/olivero/css/components/footer.pcss.css
   @@ -0,0 +1,42 @@
   +  /* @todo - #0c0d0e and #171e23 aren't currently variables */
   

   Let's create issue for this @todo and reference it here.

8. +++ b/core/themes/olivero/css/components/header-search-narrow.pcss.css
   @@ -0,0 +1,174 @@
   +  form {
   

   We could make this element of the .search-block.

9. +++ b/core/themes/olivero/css/components/header-search-wide.pcss.css
   @@ -0,0 +1,292 @@
   +.search-wide__wrapper {
   

   This could be just .block-search--wide.

10. +++ b/core/themes/olivero/css/components/header-search-wide.pcss.css
    @@ -0,0 +1,292 @@
    +.search-wide__grid {
    

    This should probably also be an element of .block-search.

11. +++ b/core/themes/olivero/css/components/header.pcss.css
    @@ -0,0 +1,94 @@
    +.header__left {
    

    Maybe there's a better name for this because this is only left when displayed on LTR.

12. +++ b/core/themes/olivero/css/components/nav-button-wide.pcss.css
    @@ -0,0 +1,104 @@
    +.nav-primary__button {
    
    +++ b/core/themes/olivero/css/components/nav-primary-button.pcss.css
    @@ -0,0 +1,132 @@
    +.primary-nav__button-toggle {
    
    +++ b/core/themes/olivero/css/components/nav-primary.pcss.css
    @@ -0,0 +1,383 @@
    +.primary-nav__menu {
    

    .nav-primary doesn't exist at the moment

13. +++ b/core/themes/olivero/css/components/nav-secondary.pcss.css
    @@ -0,0 +1,116 @@
    +  ul.menu {
    

    Can we add a class to the .menu so we don't have to nest it inside .secondayr-nav?

14. +++ b/core/themes/olivero/css/components/node-teaser.pcss.css
    @@ -0,0 +1,124 @@
    +  .field--name-field-image {
    

    Can we untie this from the image machine name?

15. +++ b/core/themes/olivero/css/components/node.pcss.css
    @@ -0,0 +1,72 @@
    +  .field--name-user-picture img {
    

    Can we untie this from user_picture field?

16. +++ b/core/themes/olivero/olivero.theme
    @@ -0,0 +1,583 @@
    +    $variables['attributes']['class'][] = 'form-type--boolean';
    

    This is in violation of BEM because .form-type block element doesn't exist.

17. +++ b/core/themes/olivero/css/components/header-search-wide.pcss.css
    @@ -0,0 +1,292 @@
    +    .icon--search {
    

    This should be an element of .block-search.

18. +++ b/core/themes/olivero/css/components/nav-secondary.pcss.css
    @@ -0,0 +1,116 @@
    +.secondary-nav__wrapper {
    

    This is not an element of the .secondary-nav because it is not inside it.

19. +++ b/core/themes/olivero/css/components/sidebar.pcss.css
    @@ -0,0 +1,55 @@
    +.region--sidebar {
    +  .menu {
    

    These shouldn't be tied together. Not sure how to best handle this.

20. +++ b/core/themes/olivero/css/components/vertical-tabs.pcss.css
    @@ -0,0 +1,100 @@
    +.vertical-tabs__menu {
    

    I think vertical-tabs would look nicer if we removed padding from this element.

@lauriii Thank you for taking the time to review the patch that we've submitted on 9/23. These suggestions are fairly straightforward and I went ahead and added them into our issues queue so that folks can start picking them up. Also, I'm assuming that we'll get additional feedback for the templates and preprocess functions soon from you as well, is that correct?

Please see all of the issues that came out from the .pcss.css code review.

• Item 1 and 2 - #3173007: [Code Review] Provide a BEM methodology to comments.pcss.css partial
• Item 3 - #3173008: [Code Review] wide image within article template a reusable component/class
• Item 4 and 5 - @mherchel what's your thought on line item 4 and 5?
• Item 6 and 7 - #3173010: Class clean up and add #0c0d0e and #171e23 as variables in Olivero's footer.pcss.css
• Item 8, 9, 10, 17 - #3173012: Olivero header-search-wide.pcss.css and header-search-narrow.pcss.css adjustments
• Item 11, 12, 13, 18 - #3173014: Address code feedback for Olivero's header.pcss.css partials
• Item 14 and 15 - #3173016: Address code feedback for Olivero's node.pcss.css partials
• Item 16 - #3173018: [Code Review] Remove .form-type as this prefix block element doesn't exist
• Item 19 - #3173022: Figure out a clean way to manage the style of Olivero's menu block in the sidebar region
• Item 20 - #3173019: [Code Review] Remove padding from .vertical-tabs__menu element

Thank you for filing follow-ups for the feedback in #78 @proeung! I don't think any of the feedback raised in that comment needs to be tagged as beta blocker since all of those changes can be done even after the beta.

@lauriii Sounds good! I just moved all of the latest feedback issues into the “stable” criteria. :)

Gábor Hojtsy pinged about a release management review.

For beta inclusion, the main thing for release management is whether there are going to be any updates or API changes required during beta. Given this is a theme, there should not really be API changes, but theme configuration changes, or other changes to core that require updates it would be good to document and ideally fix before inclusion.

One from the issue summary that sticks out along those lines: #3165970: Teaser date/time format should not be hardcoded. Would be useful to document any others in the issue summary/comments here too.

Given this is a theme, there should not really be API changes, but theme configuration changes, or other changes to core that require updates it would be good to document and ideally fix before inclusion.

The only other similar one is also in the summary above:

#3171149: Set article content type to use "wide" image style within standard profile - This one needs to be tackled only when making Olivero the default theme out of the box.

1. What should we do we theme settings that don't have an impact on Olivero? At least "User pictures in posts" and "User pictures in comments" don't seem to work.

2. +++ b/core/themes/olivero/olivero.theme
   @@ -0,0 +1,583 @@
   +      $variables['attributes']['class'][] = 'site-branding--bg-' . theme_get_setting('site_branding_bg_color');
   

   There's no block for this modifier.

3. +++ b/core/themes/olivero/templates/navigation/menu--primary-menu.html.twig
   @@ -0,0 +1,108 @@
   +    <ul {{ attributes.addClass('primary-nav__menu', primary_nav_level) }}>
   

   The primary-nav block element is missing.

4. JavaScript should be documented according to https://www.drupal.org/docs/develop/standards/javascript/javascript-api-...
5. Which browsers have you used for testing Olivero?

Didn't mean to add those tags back.

Tagging for framework manager review since we do need a sign-off from a backend framework manager as well.

1. At least "User pictures in posts" and "User pictures in comments" don't seem to work.

I think this is working properly, but I opened an issue for discussion at #3173825: "User pictures in posts" and "User pictures in comments" don't seem to work

2. There's no block for this modifier.

Opened #3173827: Correct BEM classname within site branding

3. The primary-nav block element is missing.

Opened #3173829: Correct BEM classname within menu primary

4. JavaScript should be documented according to..
Opened #3173832: Ensure Olivero's JS documentation matches standards

Which browsers have you used for testing Olivero?

We frequently test on multiple browsers. I do most of my primary development on Chrome, but also test extensively on Firefox, and Safari.

I have two virtual machines that I use to test old versions of IE11 and MS Edge. I also test high-contrast mode within these VMs.

As far as physical devices, I have a new Iphone11, and old iPad, which I test mobile Safari. I also have a Google Pixel2, which I test Android's version of Chrome.

I have not done any testing on browsers such as Samsung, or Opera.

The National Federation of the Blind (NFB) has returned their low vision accessibility test results which you can watch in a video at https://zoom.us/rec/share/G-1w9HjT0tUlWhoJnGkmlqspVFfsxSr97Dl7077fkIgGfA...

Here is a test results summary from the tester:

Like I say in the video, I was very happy with the accessibility of Olivero. There are three main things I look for when testing low vision: Contrast, Focus, and Scaling. All three of these are presented accessibly. I went through all of the user journey's that were laid out, and the accessibility stayed consistent and I was able to navigate the different elements.

And here is a breakdown of what I heard in the video:

TESTED WITH
OS/Browser: Windows Chrome and Firefox w/Mac
Mouse: Yes

OVERALL RESULT
Very happy
"Knocked it out of the park”

CONTRAST
No issues
Boring video because it’s very accessible (this is good news)

FOCUS ORDER
All makes sense
Regular reading order is set up properly
Great left to right/Top to bottom
Keyboard nav is good

FOCUS STYLE
Looks good
It’s easy to see the boxes

SCALABILITY (How well can site increase size aka ZOOM)
Hamburger menu worked on zoom and it still works
Focus order and box still look good
Contrast still good

I'll be posting the screen reader test results soon.

Adding browser testing issue to stable blocker.

Awesome! I think from my point of view the roadmap now looks good. The theme looks awesome and the code looks solid. Well done! 👏 Looking forward to working with y'all in core!

Swapping out an individual JS coding standards issue for a meta JS coding standards issue.

Some issues from @nod_'s comment in #73

1.

Olivero will be able to use the vanilla once script once it's in core, for now it's ok to leave it like that. It doesn't make sense to require jquery for such a small thing. the once feature is handled Drupal 6 style. good enough for now.

Opened #3173900: Refactor Olivero's JavaScript Drupal behaviors to use once()

2.

There are missing parameters in a few closures (scripts, navigation, second-level-navigation):

Opened #3173901: JavaScript: missing parameters in a few closures (scripts, navigation, second-level-navigation)

3.

drupalSettings is only for json objects going from the backend to the frontend

Opened #3173903: JavaScript Pass data between functions via Drupal object (instead of drupalSettings)

4.

There is a props.body.classList.remove('js-overlay-active', 'js-fixed') that should be 2 lines for IE11 compat. (only one argument supported for the remove function).

I think you might be thinking of the classList.toggle() method instead of remove(). Either way, I opened #3173905: Olivero: node.classList.remove() only supports one argument

Here's a framework manager review.

It was too hard to review using Dreditor so I applied the patch locally, and just made the changes I saw fit, as per the interdiff, then listed the rest below.

1. Firstly, let me just say how awesome this theme looks and how well it is written. Some amazing work here folks.

2. Things I found and fixed while reviewing

   1. I found a comment mark up that should have been markup so I fixed that, but it required re-flowing some of the subsequent lines
   2. I found that the theme setting schema was re-defining a number of items that were already defined in the theme_settings type, so I removed them, I couldn't find mention of schema on this issue so I assume that was not intentional (please reverse/ignore if it was)
   3. olivero_preprocess_block sets a #olivero_is_header_search_submit render property, which is picked up in olivero_theme_suggestions_input_alter. I think the code in olivero_preprocess_block should instead set '#theme_wrappers' => ['input__header_search'] and then you don't need olivero_theme_suggestions_input_alter or the whole #olivero_is_header_search_submit dance
   4. There's a lot of non type-safe string comparison in olivero.theme, I just fixed these as I saw them.
   5. There was some use of in_array() for radios and checkboxes, but another that used OR, made them consistent
   6. There was use of the \Drupal::currentUser() that I improved
   7. There were some templates mixing use of printed attributes, with the attributes object, cleaned those up

3. Things that need fixing

   1. Must-have

      Olivero doesn't have security opt-in, so its ok to discuss this in public in my book. The template file menu--primary-menu.html.twig has an XSS vulnerability through the use of the rawfilter. Using that is always a red-flag, if you feel you need to reach for it, don't 😁.
      

      If I enter markup in menu titles

      

      It gets rendered

      

      If I enter javascript

      

      it gets executed

   2. Should-have

      The template file field--comment-body.html.twig looks to largely duplicate field.html.twig with just a different class, can we do that using include/extend/embed to avoid duplication? It looks like the class being added is already added by olivero_preprocess_field so the whole file may now be redundant.

   3. Must-have

      The template book-all-books-block.html.twig exists twice, once in the navigation folder and again in the blocks folder. We should remove one. Similarly for book-tree.html.twig

   4. Must-have

      The code in olivero_theme_suggestions_field_alter is too generic to enforce the template field--type-entity-reference--formatter-entity-reference-label.html.twig for all entity-reference fields using the label formatter, it uses class names like field--tag, so appears to be specific to the tags field, we need to use a less general template suggestion so it only applies for the tags field

   5. Should-have

      The template menu-local-task.html.twig contains invalid HTML, div isn't allowed inside a button - we should endeavour to run markup from the theme through the W3C validator to prevent a flood of core-issues about invalid markup 😅

   6. Should-have

      olivero_preprocess_node hard-codes the date format, we should ship a new date format in the theme's config so this can be modified easily by site owners. Similarly for olivero_preprocess_search_result

   7. Must-have

      Both olivero_theme_suggestions_block_alter and olivero_preprocess_block try to load the block by ID, but this should already be available as $variables['#block'] as it is added by \Drupal\block\BlockViewBuilder::buildPreRenderableBlock

   8. Should-have

      The comment on olivero_preprocess_menu_local_tasks no longer seems relevant, there's no use of #attached in the function body.

   9. Should-have

      There is no indentation to indicate a comment is a reply to another comment - is that intended? Given there is a file comments.es6.js, I assume not - as that functionality looks to be intended to collapse replies.
      

      In this image, the second comment is a reply to the first one

4. Questions

   1. The theme comes with no layouts - should we be shipping some Olivero specific layouts for Layout Builder integration? This can be a follow-up.
   2. scripts.es6.js has this comment: // @todo, I'm not sure we even need the .mobile-buttons container anymore. - can that be resolved / removed?

5. Observations/Notes to self

   1. The metropolis font is public domain and the lora font is OFL, both of which are GPL compatible according to https://www.gnu.org/licenses/license-list.html
   2. I've seen issues with autoloading having classes in {a theme folder}/src in certain environments, it seems to be more problematic when the admin theme is a different theme - \Drupal\olivero\OliveroPreRender might have issues with this, its one to watch.

Sorry uploaded the wrong file instead of the interdiff

1. +++ b/core/themes/olivero/olivero.theme
   @@ -153,7 +155,7 @@ function olivero_theme_suggestions_form_alter(array &$suggestions, array $variab
   -  if ($variables["element"]["#field_type"] == 'entity_reference' && $variables["element"]["#formatter"] == "entity_reference_label") {
   +  if ($variables['element']['#field_type'] == 'entity_reference' && $variables['element']['#formatter'] == 'entity_reference_label') {
   

   I missed another use of == instead of === here, can this be fixed in next pass.

2. +++ b/core/themes/olivero/olivero.theme
   @@ -532,10 +525,9 @@ function olivero_preprocess_field__comment(&$variables) {
   +  if ($user->isAuthenticated() && $user instanceof UserInterface) {
   

   FYI the instanceof here is super defensive, happy if you want to remove it, its unlikely someone would swap out the User class with something that's not a content entity.

Part 2 of the review, test coverage

Here's things in the .theme file I think we should have tests for, as they are likely to break if there are array structure changes.

1. olivero_preprocess_block
2. olivero_theme_suggestions_form_alter
3. olivero_form_alter
4. olivero_preprocess_input
5. olivero_preprocess_field_multiple_value_form
6. olivero_preprocess_field__comment

These would be stable blockers in my book

replies to @larowlan's part one review in #98

1. Thanks! 😊
2. I appied all of your fixes downstream into the Olivero repo in a series of attributed commits. Thanks!

3. Things that need fixing

   1. XSS vulnerability:

      Opened #3174065: XSS vulnerability in menu--primary-menu.html.twig

   2. The template file field--comment-body.html.twig looks to largely duplicate field.html.twig ...

      Opened #3174067: Avoid duplication in field--comment-body.html.twig

   3. The template book-all-books-block.html.twig exists twice...

      Opened #3174069: book-all-books-block.html.twig and book-tree.html.twig exist twice

   4. The code in olivero_theme_suggestions_field_alter is too generic...

      Opened #3174070: olivero_theme_suggestions_field_alter is too generic to enforce template

   5. The template menu-local-task.html.twig contains invalid HTML...

      Opened #3174074: menu-local-task.html.twig contains invalid HTML

   6. olivero_preprocess_node hard-codes the date format...

      This is a known issue: #3165970: Teaser date/time format should not be hardcoded

   7. Both olivero_theme_suggestions_block_alter and olivero_preprocess_block try to load the block by ID

      Opened #3174075: Both olivero_theme_suggestions_block_alter and olivero_preprocess_block should NOT load block by ID,

   8. The comment on olivero_preprocess_menu_local_tasks no longer seems relevant, there's no use of #attached in the function body.

      Opened #3174101: The comment on olivero_preprocess_menu_local_tasks no longer seems relevant

   9. There is no indentation to indicate a comment is a reply to another comment...

      I think this isn't correct, but opened #3174077: There is no indentation to indicate a comment is a reply to another comment to document or fix.

4. Questions

   1. The theme comes with no layouts - should we be shipping some Olivero specific
      layouts for Layout Builder integration? This can be a follow-up.

      We override the stylinig of the core layouts. See https://git.drupalcode.org/project/olivero/-/blob/8.x-1.x/olivero.info.y...

   2. scripts.es6.js has this comment: // @todo, I'm not sure we even need the .mobile-buttons container anymore. - can that be resolved / removed?

      Opened #3174088: Remove .mobile-buttons comment within Olivero's scripts.es6.js

Updating summary putting @larowlan's review's "must haves" as beta-blockers.

Moving "Should have" items to "stable" blockers within the summary.

Adding failing tests #3174105: Fix tests for Olivero in core patch as beta blocker.

Adding additional test coverage #3174107: Add additional testing coverage for Olivero as stable blocker.

Adding #3174070: olivero_theme_suggestions_field_alter is too generic to enforce template as a beta blocker as it is listed as "must have" in @larowlan's review.

just 5c, additionally to comment replies/hierarchies there's related to UX of comments #169938: Usability: Configurable comment link on teasers

I moved #3165970: Teaser date/time format should not be hardcoded to must-have since if it's committed after the theme, we'd need an update function for sites that have Olivero installed.

New patches attached. The main purpose is to verify that the tests pass as part of #3174105: Fix tests for Olivero in core patch.

Note that the interdiff is actually larger than the patch because of all the CSS refactoring in #3172452: META: Convert codebase to use postcss-preset-env.

Many improvements (yet still a couple beta-blockers left) including:

• #3174065: XSS vulnerability in menu--primary-menu.html.twig
• #3172452: META: Convert codebase to use postcss-preset-env
• #3174067: Avoid duplication in field--comment-body.html.twig
• #3174069: book-all-books-block.html.twig and book-tree.html.twig exist twice
• #3173903: JavaScript Pass data between functions via Drupal object (instead of drupalSettings)
• #3172225: Toolbar menu inaccessible when javascript disabled
• #3165452: Override Drupal.theme.message to to make sure JS messages get rendered correctly
• #3152382: Radios and checkbox not aligned with label in settings tray
• Various fixes by @larowlan within the his last review
• Custom selection styling
• Replacing screenshot with an actual screenshot (as opposed to render)
• Nav chevnron alignment issues

Adding #3048848: Syndicate block outputs wrong feed URL as a related Drupal core issue.

New patch to test the automated tests 🤞

Drive-by comment after commenting on #3175051: See if we can leave the block around in BlockViewBuilder::preRender so front-end themes can make use of it. Sorry if that has been discussed already, just my opinion as a non-frontend person who wasn't involved in the process (I know, annoying).

There seem to be quite a few preprocess and theme suggestion functions that are pretty opinionated/hardcoded for a core theme and/or are fixing things that should maybe instead actually be done in the modules adding those things? Most might be acceptable to get in, but maybe could have follow-ups to clean up or so?

Some examples:
* If those block theme suggestions are useful for this theme then they might be useful for everyone and we could consider adding them by default? That would resolve one possible reason why themes need to load the block entity. FWIW, adding new theme suggestions by default can influence custom ones and the order can be tricky too, so BC needs to be considered. It's also not free to have a lot of theme suggestions.
* shortcut customizations, it's exceptionally rare to have shortcuts in frontend theme enabled imho but if something like not-showing-it-on-frontpage is a thing, maybe that should be done in shortcut.module? as an option? not sure.
* Overriding the date variable for the node template/preprocess with a custom one provided by the theme seems a bit unusual (again, if such a format is useful, add it to standard?) and also goes against the work to make things like author/label/date variables not hardcoded. See #3015623: [PP-many] Remove outdated code relating to "Expose Title in Manage Display". I know it's in a weird middle state right now with theoretically being configurable but it requires some other module to flag it as such first, but we should at least have a plan on how to complicate the very slow and tedious work around improving that further.

New patch. The purpose of this is primarily to check that the automated tests pass #3174105: Fix tests for Olivero in core patch

🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞
🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞
🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞
🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞
🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞🤞

🙌

index 0000000000..9ae8bbaa62
--- /dev/null
+++ b/core/themes/olivero/config/schema/olivero.schema.yml
@@ -0,0 +1,23 @@
+# Schema for the configuration files of the Olivero theme.
+
+olivero.settings:
+  type: theme_settings
+  label: 'olivero settings'
+  mapping:
+    third_party_settings:
+      type: mapping
+      label: 'Third party settings'
+      mapping:
+        shortcut:
+          type: mapping
+          label: 'Shortcut'
+          mapping:
+            module_link:
+              type: boolean
+              label: 'Module Link'
+    mobile_menu_all_widths:
+      type: integer
+      label: "Mobile menu all widths"
+    site_branding_bg_color:
+      type: string
+      label: "Site branding background color"

Is this configuration finalized or are any issues likely to require updating it?

Same question for the block config that ships in config/install - is any of this likely to change during beta?

Is this configuration finalized or are any issues likely to require updating it?

We don't foresee any issues that will necessitate an update. However, we do want to commit the following patch that will include a theme setting that will be an additional setting to the schema. That won't require any updates, however. We hope to add this post-beta.

#3174774: Enable option to "sticky" the sidebar

After talking with @Gábor Hojtsy in Slack, we need to stop introducing new features and keep the patch static (except for necessary bug fixes).

With that in mind, attached is [hopefully] the final patch prior to the initial core commit. This patch includes several bug, visual, code quality, and accessibility fixes. This patch also reverts the new "sticky sidebar" feature in #3174774: Enable option to "sticky" the sidebar, which we hope to add in as a post-beta feature at a future date.

Also note that this patch is generated by core's compilation script (as opposed to Olivero's) that does not include the pxtorem PostCSS plugin (which we hope to add in #3117698: Allow PostCSS Plugin “Px to Rem” in core for Olivero theme), so the interdiff is larger.

Also, please let me know if you want additional interdiffs, and I'll get em up ASAP.

That won't require any updates, however.

From that issue:

diff --git a/config/install/olivero.settings.yml b/config/install/olivero.settings.yml
index 11f0599..bae1e24 100644
--- a/config/install/olivero.settings.yml
+++ b/config/install/olivero.settings.yml
@@ -12,4 +12,5 @@ favicon:
   use_default: true
 mobile_menu_all_widths: 0
 site_branding_bg_color: blue
+sticky_sidebar: 1
 debug: 1

This changes the default configuration - won't sites that have Olivero installed end up with a config diff (i.e. + sticky_sidebar: 0? And is it an intentional decision to not update to match the new default config for those sites?

I think there are three options:

1. Add it back to the patch (no update) to commit before beta
2. Commit it later with a post update to set the default to 0
3. Commit it later with a post update to set the default to 1

Since themes can't provide update functions, the update would have to happen in system module checking the existence of the theme and config. There's also the question of whether we'd want to add a system post update for an experimental theme at all, which might mean deferring that feature until Olivero is stable and adding it in a minor release afterwards.

I took it out of the patch after speaking with @Gábor Hojtsy. We were worried that adding that new feature into the theme would require re-reviews (on a very short timeline!).

We could easily generate a new patch to add it in, however, this patch needs to get in before Friday, and I don't want to create new time-sensitive work for reviewers.

Thoughts on this?

I also want to note that this is a somewhat minor feature that we could just omit from the final release of Olivero.

We were worried that adding that new feature into the theme would require re-reviews (on a very short timeline!).

Adding it after Olivero is committed means we'll need to write an update function + test coverage, that runs on every Drupal install regardless of whether Olivero is installed or not (albeit with an early return if it isn't) - since themes can't provide their own updates. So it's preferable from a release management/stability perspective to either add it now before commit, or much later once Olivero is stable, but not so much in-between.

Makes sense. Let's defer this until much later (once Olivero is stable), so we can avoid any need for re-reviews. I'm marking #3174774: Enable option to "sticky" the sidebar as postponed.

OK I haven't reviewed all the code here, but I think that's everything from an API change/data integrity perspective either dealt with prior to commit or postponed to after stable now, so untagging for release manager review.

Per @Gábor Hojtsy, it's up to the Olivero team to do a final RTBC now that the "needs" tags are removed.

I took one last look through the code, and the theme is in a really good place. Excited to get this in!

On top of people already listed in this issue, the following people have issue credits for Olivero and should be credited. This is sorted by commit count.

• mherchel
• proeung
• kostyashupenko
• hansa11
• brianperry
• larowlan
• sd9121
• q0rban
• nitesh624
• himanshu_sindhwani
• thejimbirch
• mrconnerton
• Sreenivas Bttv
• boulaffasae
• andrewozone
• jerseycheese
• rahulrasgon
• rabbitlair
• poojakural
• Dom.
• shimpy
• Indrajith KB
• Maithri Shetty
• shaktik
• kiran.kadam911
• alexdmccabe
• DuneBL
• Gábor Hojtsy
• hawkeye.twolf
• komalkolekar
• keboca
• nod_

We need to add drupal/olivero to core/composer.json - we need "drupal/olivero": "self.version", in the replace section. After adding it you need to run composer update drupal/core in the root directory to ensure that composer.lock is updated correctly.

I guess we can do this in a follow-up but normally this is included when we add something. Following the above instructions will show that the the recent frontmatter library addition didn't do it correctly... so maybe follow-up is fine /shrug.

@alexpott brought up the fact that I only added Olivero committers for issue credit, which is incorrect. We want all the contributors to get credit. Here is the additional names to add (sorry!)

• alexpott
• ambuj_gupta
• andrewmacpherson
• andriyun
• andypost
• anevins
• bash247
• chetanbharambe
• CocoM
• ellenoise
• hussainweb
• JayKandari
• jhodgdon
• jponch
• ju.vanderw
• jwitkowski79
• KarenS
• KarinG
• katannshaw
• Lal_
• lauriii
• Lokender Singh2
• MaxPah
• mrinalini9
• msuthars
• Pooja Ganjage
• pradeepjha
• Ramya Balasubramanian
• ressa
• Sebacic
• shaal
• sharma.amitt16
• sonam.chaturvedi
• steinmb
• tanmaykadam
• thedrupalkid
• trebormc
• Ujval Shah
• vebrovski
• viappidu
• vinitk
• volkswagenchick
• Webbeh
• Yuri

I'm going to work on the composer changes that @alexpott mentioned in #164 shortly, expect a new patch and interdiff in a bit!

Updated patches are attached with composer changes.

Note that composer made some changes that I didn't expect in composer/Metapackage/CoreRecommended/composer.json.

diff --git a/composer/Metapackage/CoreRecommended/composer.json b/composer/Metapackage/CoreRecommended/composer.json
index 39e3d9b6d0..714094710f 100644
--- a/composer/Metapackage/CoreRecommended/composer.json
+++ b/composer/Metapackage/CoreRecommended/composer.json
@@ -13,6 +13,8 @@
         "doctrine/annotations": "1.10.4",
         "doctrine/lexer": "1.2.1",
         "doctrine/reflection": "1.2.1",
+        "drupal/core-project-message": "9.1.x-dev",
+        "drupal/core-vendor-hardening": "9.1.x-dev",
         "egulias/email-validator": "2.1.19",
         "guzzlehttp/guzzle": "6.5.5",
         "guzzlehttp/promises": "v1.3.1",

Not sure if this is correct or not. Either way, patch + interdiff is attached.

@mherchel ah you must of run the composer commands while on a branch - you need to prefix the commands with COMPOSER_ROOT_VERSION=9.1.x-dev - i should have written the commands this way - sorry.

Patched attached:

• Fixes all spelling errors detected by cspell. The only bit I ummed and ahhed over was adding navs to the dictionary.
• Fixes incorrect CSS order according to stylelint
• Fixes other stylelint issues
• Renames block config so it passes spelling checks
• Fixes composer stuff
• Fixes spaceless to use the new apply spaceless thing
• Fixes incorrect file modes - files should always be 644

Leaving at RTBC because all of this is the result of automated checks.

+++ b/core/themes/olivero/css/layout/grid.css
@@ -10,6 +10,8 @@
+/* stylelint-disable unit-whitelist */
+

This is because these files are using the fr unit. Which is not in our list of allowed units. We should open a follow-up to discuss adding it.

Patch looks good! Thanks for working on this!

Opened follow up issue #3176589: Add FR units to list of CSS allowed units.

I added #3117698: Allow PostCSS Plugin “Px to Rem” in core for Olivero theme to a beta blocker for now because of it's impact to accessibility. Alternative solution would be to convert the source to use rem values where applicable.

1. +++ b/core/themes/olivero/css/components/book.pcss.css
   @@ -0,0 +1,109 @@
   +.book-navigation {
   +  & .menu {
   

   We should make the book navigation markup and CSS in Olivero follow BEM.

2. +++ b/core/themes/olivero/olivero.theme
   @@ -0,0 +1,576 @@
   +      $variables['items'][$key]['content']['#image_style'] = 'wide';
   

   If the image style doesn't exist, there's an exception. Given that, I don't know if the current priority of #3171570: Remove Olivero's custom hard-coding of the image style within article content type's full view mode is correct.

3. We have instructions for how the theme screenshots should be generated. We should update the screenshot with one that has been created using the steps documented there. If the instructions are out of date, we should probably open an issue to update the instructions and re-generate all of the theme screenshots using the new steps.

4. +++ b/core/themes/olivero/olivero.info.yml
   @@ -0,0 +1,85 @@
   +description: 'A clean, modern, and accessible Drupal frontend theme.'
   

   Wondering if we should use something else than modern for describing the theme. Based on https://www.lexico.com/definition/modern, it is an adjective referring to the present or recent times as opposed to the remote past. This is true now but this wouldn't necessarily be true in 10 years.

5. +++ b/core/themes/olivero/olivero.info.yml
   @@ -0,0 +1,85 @@
   +  content_below: 'Content Below (Flex Layout)'
   +  footer_top: 'Footer Top (Flex Layout)'
   

   Is there a less technical term than flex layout? This is displayed in the UI and I could imagine that non-technical users could find this confusing.

6. Olivero should have default block configuration for local actions. Not having a block in place for that could end up making some functionality not being available through the UI.

7. +++ b/core/themes/olivero/config/schema/olivero.schema.yml
   @@ -0,0 +1,23 @@
   +    site_branding_bg_color:
   +      type: string
   +      label: "Site branding background color"
   

   What are you planning to do with this configuration if Olivero ends up supporting color module in #3086514: Investigate use of the changing color themes for Olivero?

8. +++ b/core/themes/olivero/templates/field/field--comment.html.twig
   @@ -0,0 +1,57 @@
   + * Default theme override for comment fields.
   
   +++ b/core/themes/olivero/templates/form--search-block-form.html.twig
   @@ -0,0 +1,15 @@
   + * Default theme implementation for a 'form' element.
   
   +++ b/core/themes/olivero/templates/form/field-multiple-value-form.html.twig
   @@ -0,0 +1,50 @@
   + * Default theme implementation for an individual form element.
   
   +++ b/core/themes/olivero/templates/menu-local-action.html.twig
   @@ -0,0 +1,17 @@
   + * Default theme implementation for a single local action link.
   
   +++ b/core/themes/olivero/templates/navigation/toolbar.html.twig
   @@ -0,0 +1,51 @@
   + * Default theme implementation for the administrative toolbar.
   
   +++ b/core/themes/olivero/templates/user/user--compact.html.twig
   @@ -0,0 +1,25 @@
   + * Default theme implementation to present all user data.
   
   +++ b/core/themes/olivero/templates/user/username.html.twig
   @@ -0,0 +1,31 @@
   + * Default theme implementation for displaying a username.
   
   +++ b/core/themes/olivero/templates/views/views-view--frontpage.html.twig
   @@ -0,0 +1,94 @@
   + * Default theme implementation for main view template.
   

   These are theme overrides.

9. +++ b/core/themes/olivero/templates/form/fieldset.html.twig
   @@ -0,0 +1,81 @@
   + * @file
   

   We should add available variables to the documentation.

10. +++ b/core/themes/olivero/templates/includes/get-started.html.twig
    @@ -0,0 +1,45 @@
    + * TODO:
    + * - Move this markup into Drupal's core Frontpage Views.
    + * - Translate/localize this page for different languages.
    

    Let's reference issue for this todo.

11. +++ 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.

12. +++ b/core/themes/olivero/templates/layout/html.html.twig
    @@ -0,0 +1,57 @@
    +    {{ noscript_styles }}
    

    Let's document this in the @file documentation.

13. +++ b/core/themes/olivero/templates/block/block--secondary-menu--plugin-id--search-form-block.html.twig
    @@ -0,0 +1,43 @@
    +<div{{ attributes.addClass(classes) }}>
    +  {{ title_prefix }}
    ...
    +    <h2{{ title_attributes }}>{{ label }}</h2>
    ...
    +  {{ title_suffix }}
    

    These are missing from the available variables list.

14. +++ b/core/themes/olivero/templates/block/block--system-powered-by-block.html.twig
    @@ -0,0 +1,14 @@
    +{% extends "block.html.twig" %}
    

    Missing @file documentation.

15. +++ b/core/themes/olivero/templates/content/media.html.twig
    @@ -0,0 +1,28 @@
    +    not media.isPublished() ? 'media--unpublished',
    

    media is missing from the available variables list.

16. +++ b/core/themes/olivero/templates/content/node--article--full.html.twig
    @@ -0,0 +1,5 @@
    +{% include '@olivero/content/node.html.twig' with
    

    Missing @file documentation.