phenaproxima / starshot-prototype

Prototype of a new kind of Drupal, based on recipes and loaded up with contrib's best modules and themes. Not a fork or a distribution.
https://drupal.org/starshot
104 stars 36 forks source link

Add Easy Breadcrumb module #87

Closed malabya closed 2 weeks ago

malabya commented 1 month ago

Adding a PR based on #85

phenaproxima commented 1 month ago

I don't object to this PR in principle but would like another opinion first from someone who actually builds sites for clients in their day job. :) Assigning to @pameeela for that, but I'd also love to hear @thejimbirch's thoughts.

pameeela commented 1 month ago

Definite +1 to this, it solves a major Drupal wtf in appending the current page to the breadcrumb (in addition to some other goodies), and AFAIK is the simplest way to do this. So I'd be happy to merge, just pending the question about the config. Definitely some of it is the module defaults so I think it needs a tweak.

thejimbirch commented 1 month ago

Yes! Agree 100% with the request and @pameeela

But the config is a copy paste of the config from the module. That needs to be removed and some changes made with config actions.

I will grab some best practices from some of our projects and comment back.

phenaproxima commented 1 month ago

No objections from me, but I'll hold off on merging this until @thejimbirch comments back with the best practices.

thejimbirch commented 1 month ago

Looking at this a little closer this morning. Just enabling and configuring the module is not enough. We need to also make sure the breadcrumb block appears on pages. In recipes/starshot/recipe.yml You want to add the block to the Olivero config import section since the recipe is selectively adding Olivero blocks.

config:
  import:
    olivero:
      - block.block.olivero_breadcrumbs.yml

You don't need to do this for the Gin admin theme as all blocks are being imported there.

Here are the minor settings changes, with comments explaining why. Feel free to remove those if you want.

@pameeela, you implied that "appending the current page to the breadcrumb" is bad. It is specified in the Google Search Gallery documentation for Breadcrumbs (Last updated 2024-04-09 UTC). We usually include it for best practices. Do you have a site where you have configured the perfect breadcrumb and can share the config?

Here are the tweaks we usually make:

config:
  actions:
    easy_breadcrumb.settings:
      simple_config_update:
        # Add Schema.org Breadcrumb markup.
        add_structured_data_json_ld: true
        # Remove breadcrumbs when only home appears.
        # Since we print the page title, this is only the home page.
        hide_single_home_item: true
pameeela commented 1 month ago

@thejimbirch ah no, it is definitely good, I was meaning that this module provides that functionality, which AFAIK is not in core? (The not being in core is the wtf.) Obviously you can alter it but we usually use this module for it since it gives you a bit more.

thejimbirch commented 1 month ago

Ok, thanks for the clarification!

malabya commented 1 month ago

@phenaproxima @thejimbirch Updated the PR to add the config updates via actions.

phenaproxima commented 2 weeks ago

Needs to be rebased against main. Let's also move the requirement for drupal/easy_breadcrumb into recipes/starshot/composer.json, as of #109.

phenaproxima commented 2 weeks ago

Gave this a quick manual test and yes, this is a no-brainer! @pameeela and @thejimbirch feel good about it and so do I. Cheerfully merged!