Pager should use single directory components

Initial pass at converting the pager for sdc

Addressed feedbacks from #6, attached interdiff for same. please review

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Tried to fix failures in #8.

Using this MR: https://git.drupalcode.org/project/drupal/-/merge_requests/8768 Comment !12
In this scenario, I have created two nodes and a view to show a node using a pager.
After saving the view go to the page URL and show the Output like that
Screenshot Attached:

To See this Output May be Moved to RTBC
Thanks.

One question on the MR.

Looks like the library definition still needs updating too.

Appears to have multiple test failures.

First-time contributor - Refactoring the pager component
Hi, this is my first time contributing to Drupal. 😊

This patch includes updates to the pager.component.yml and pager.twig files
If there’s anything I’ve done incorrectly or any feedback you have, please let me know. Thank you!

Hello and thanks for the first time contribution.

So with all issues they should use the standard issue template, the template that's there when you create a new ticket. Even if sections aren't relevant like API change leave it and put a NA under it.

Will probably need SDC maintainer eyes, will ping them.

Hi Ignacio,

Thanks for your work. Here ara a few feedbacks from a first quick look about Pager component: https://git.drupalcode.org/project/drupal/-/merge_requests/8768/diffs

Undefined objects

This is not enough to be understood by developers using your component and leveraged by display builders (UI Patterns 2, the upcoming Experience Builder...):

        previous:
          type: object
          title: Previous page
          description: Represents the previous page relative to the current page, typically with a link to the preceding page in the pager.

You need to tell what is inside the object, which properties.

Buy the way, don't feel obligated to mimic the current Twig logic and model of core/themes/olivero/templates/navigation/pager.html.twig , this is also the occasion of proposing a cleaner model and do the mapping when you include the component.

Alien classes

Are you sure layout--content-medium belongs to the component template ?

attributes.addClass(['pager', 'layout--content-medium'])

it looks foreign for me.

Maybe you can inject it from the presenter template instead:

{% include 'olivero:pager' with {
  attributes.addClass('layout--content-medium'),

Use include function instead of include tag

According to Twig documentation, it is recommended to use the include function instead as it provides the same features with a bit more flexibility: https://twig.symfony.com/doc/3.x/tags/include.html

Proper use of icons

It is a bit weird to use componentMetadata as a workaround to get icon path but I understand why you did that:

 {{ include (componentMetadata.path ~ '/images/pager-previous.svg') }}

However, there is a new Icon API in Drupal Core since Drupal 11.1: #3471494: Add an icon management API

So, you can add an icon pack in olivero.icons.yml (naive, non tested, example):

olivero:
  label: "Olivero icons"
  extractor: svg
  config:
    sources:
      - path/to/your/icons/
  settings: {}
  template: >-
    <svg {{ attributes }}>{{ content }}</svg>

Adn then call your icon from your template:

 {{ icon ('olivero', 'pager-previous') }}

It is a new API. I would be happy to help if necessary.

Edited MR. Attempt to make code more comprehensible.

Hi Pierre,
Thank you for your review and comments—I really appreciate the feedback.
I’ve made some updates based on the suggestions:
1. I added a new component (`pager_item`) and structured the render arrays in the preprocess function. This seemed necessary because of the complexity, but I’m not entirely sure if this was the best approach. Please let me know if there’s room for improvement.
2. I also added the view mini pager template since it was previously unstyled.

Unfortunately, I wasn’t able to make the icons work. Any guidance or feedback on this would be greatly appreciated.

Lastly, thank you, Andrew, for reviewing the text as well. English isn’t my first language, and I’d really appreciate another review of the text if possible.

Please let me know if I can improve or update the code further—I am eager to learn, so don’t hesitate to share any suggestions.
Thanks again!

Hi Ignacio,

Undefined objects

I added a new component (`pager_item`) and structured the render arrays in the preprocess function. This seemed necessary because of the complexity, but I’m not entirely sure if this was the best approach. Please let me know if there’s room for improvement.

Creating a pager_item component is not a bad idea. There is plenty of situations when we want a "sub component" repeated in a parent component' slot:

• slides in a slider component
• items in an accordion component
• tab in a tabs component

However, in this case, I don't believe it is the best choice:

• there is not slot in pager_item so the repeated data structure can be expressed with JSON schema
• this data structure already exists in Drupal: pager links. it just need to be defined properly instead of a simple type: object

Also, if you decide to keep pager_item, the BEM HTML class name must not be an element of pager:

{% set classes = [
  'pager__item',
  'pager__item--' ~ itemType
] %}

but its own block:

{% set classes = [
  'pager-item',
  'pager-item--' ~ itemType
] %}

See: https://getbem.com/naming/

Proper use of icons

Unfortunately, I wasn’t able to make the icons work. Any guidance or feedback on this would be greatly appreciated.

This new API (available since Drupal 11.1) is easy to use once the icon pack is defined.

First, you need to have a clearer organisation of your icons. You actually have:

• images/icons/pager-first.svg (without the SVG wrapper, that's weird, why?)
• images/icons/pager-previous.svg (without the SVG wrapper, that's weird, why?)
• components/paginator/pager_item/images/pager-first.svg
• components/paginator/pager_item/images/pager-previous.svg

I have no opinion about where to keep those 2 icons, but it seems there is some duplications to fix, so I am proposing to move components/paginator/pager_item/images/*.svg to images/icons/*.svg , replacing the existing files.

Then you can create a olivero.icons.yml file at the root of the theme with:

olivero:
  label: "Olivero"
  extractor: svg
  config:
    sources:
      - images/icons/*.svg
  settings: {}
  template: >-
    <svg {{ attributes }}>{{ content }}</svg>

And try to print {{ icon ('olivero', 'pager-previous') }} in a template. Is it working?

If yes, it is a good start :) If no, let's check this together.

Linting & and checking

There is no official Twig linter for Drupal Core yet, but you can run the Twig files of the components added by you (and only those ones) with #3284817: Adopt vincentlanglet/twig-cs-fixer for Twig coding standards

Also, https://www.drupal.org/project/sdc_devel can be useful to check your component (definition & template) :

• there is a report page: /admin/reports/ui-components
• and a drush command: drush sdv olivero

The Olivero theme was approved for removal in #3590816: [policy, no patch] Deprecate Olivero and move to contrib.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3595082: [meta] Tasks to deprecate the Olivero theme and the removal work in #3595085: [meta] Tasks to remove the Olivero theme.