🇺🇸United States @mherchel

Going to try to fix tests before I put out a release. Mid-week by latest

Merged! Will tag a release shortly!

Works great for me! Thank you

Whoohoo!! Thanks Matt and Lauri!!!

I'm hoping that this can be backported to 10.5.x. As it stands now if I want to make a theme that supports both D10 and D11, I still have to work around this bug by adding additional specificity onto my SDC CSS selectors. :D

I gave a quick look at the markup and CSS and added a couple suggestions. Everything is looking great at first glance although I haven't yet tested it (will probably do so next week).

Thanks for working on this!

I'm also crediting @caligan here, since she worked on it at the DrupalCon Atlanta sprints (although it looks like work was duplicated with @javi-er, which happens).

Re-tested. Confirmed new method outputs the same. Not including screenshots because they're exactly the same as in #59 and #62

Reviewing now!

Image showing issue is fixed with aggregation

Confirmed that this is fixed when aggregation is enabled. I tested this by modifying Olivero's CSS in two places:

1. In the teaser SDC component.
2. In the theme's base CSS.

The SDC styles take get injected after, which takes precedent, which is the correct behavior! YAY!!!

Here's a picture of Matt doing the work!! :D

Screenshots showing that the issue is fixed. I'm going to double check this works as expected when aggregation is enabled.

Draft CR at https://www.drupal.org/node/3515838 → . Still needs screenshots, which I will add as I test this out.

mherchel → changed the visibility of the branch 3374901-css-load-order to hidden.

mherchel → changed the visibility of the branch 11.x to hidden.

mherchel → changed the visibility of the branch 3374901-sdc-stylesheets-are to hidden.

Assigning credit (everyone gets credit)

Updating issue summary.

Updating title.

mherchel → created an issue.

I tested this by using the following snippet in the page.html.twig

              {% embed 'drupal_cms_olivero:accordion' with {
                title: 'This is the main heading',
                description: 'This is a long description.This is a long description.This is a long description.This is a long description. ',
              } only %}
                {% block items %}
                  {% embed 'drupal_cms_olivero:accordion-item' with {
                    title: 'Accordion item headingAccordion item headingAccordion item headingAccordion item headingAccordion item heading',
                  } only %}
                    {% block accordion_content %}
                    Lorem ipsum dolor sit amet consectetur adipisicing elit. Nam, veniam, sint facilis pariatur hic iste dignissimos rerum possimus, totam architecto qui quos molestiae incidunt voluptatibus fugiat omnis nihil rem eum!
                    {% endblock %}
                  {% endembed %}

                  {% embed 'drupal_cms_olivero:accordion-item' with {
                    title: 'Accordion item heading',
                  } only %}
                    {% block accordion_content %}
                    Lorem ipsum dolor sit amet consectetur adipisicing elit. Nam, veniam, sint facilis pariatur hic iste dignissimos rerum possimus, totam architecto qui quos molestiae incidunt voluptatibus fugiat omnis nihil rem eum!
                    {% endblock %}
                  {% endembed %}

                  {% embed 'drupal_cms_olivero:accordion-item' with {
                    title: 'Accordion item heading',
                  } only %}
                    {% block accordion_content %}
                    Lorem ipsum dolor sit amet consectetur adipisicing elit. Nam, veniam, sint facilis pariatur hic iste dignissimos rerum possimus, totam architecto qui quos molestiae incidunt voluptatibus fugiat omnis nihil rem eum!
                    {% endblock %}
                  {% endembed %}

                  {% embed 'drupal_cms_olivero:accordion-item' with {
                    title: 'Accordion item heading',
                  } only %}
                    {% block accordion_content %}
                    Lorem ipsum dolor sit amet consectetur adipisicing elit. Nam, veniam, sint facilis pariatur hic iste dignissimos rerum possimus, totam architecto qui quos molestiae incidunt voluptatibus fugiat omnis nihil rem eum!
                    {% endblock %}
                  {% endembed %}

                {% endblock %}
              {% endembed %}

This should be good to go!

Overall this is looking good. There's a couple issues:

1. The content area within the accordion items should be a slot, since it will accept arbitrary markup (and possibly other components)
2. We should be using CSS nesting
3. Need to use CSS logical properties for proper RTL support

I'm in the DrupalCon contrib room right now, so going to work on this.

wim leers → credited mherchel → .

I'm of the opinion that we need to let people do whatever if they access this from the code/render arrays directly, so enforcing would be only at the UI level in the various display builders interfaces.

I agree with this.

I'm just seeing a lot of complexity for a feature that will get in the way of the ambitious site builder (which is our focus, or marketers in the case of Drupal CMS)

This functionality would radically simplify the page building experience for site builders / content editors / marketers by removing a significant amount of cognitive load, and removing the need for custom documentation.

I'm personally indifferent if the suggestions/restrictions are created at the SDC or in config, just as long as it's possible for the theme to ship the suggestions/restrictions.

What do you think?

It really depends on how XB and equivalents implement the UI.

I want XB to 100% limit accordion_group to only accept accordion_item SDCs. The key suggested seems somewhat weak for this action.

It also might be worth exploring whether we can limit an SDC to only appear in a specific slot of a specific SDC. A use case of this is that I don't want my end user to be able to place an accordion_item in the main content area. It only works when placed inside of accordion_group

mherchel → created an issue.

The following component placed in my theme's components directory is what breaks it.

mherchel → created an issue.

Whats holding this back

Pretty sure we need some comprehensive tests.

Since my D6 days, the frontend has always been the stepchil

I hear that. But since D10, we've made a lot of improvements including a better dump(), UI for enabling twig debugging, some great new twig filters, and of course SDC

but we need a theme too (to glue it all together)

That's in progress now. See https://www.drupal.org/about/starshot/blog/announcing-the-selected-partn... →

probably the navigation/focus handling needs to be extracted/provided as a own navigation module which then can be integrated/combind in any theme.

Not sure this is possible. Obviously focus states are important, but so is order. But yeah, I hear you.

CSS layers won't work because all declared layers will be less specific than styles with no declared layer (which is the rest of Drupal).

Web components would be ideal, because encapsulation is one of the main benefits. Not sure of @finnsky’s MR is enough though, as far as I can tell it just protects the button. It should also protect everything else there (list styles etc). From the Slack thread at https://drupal.slack.com/archives/C7AB68LJV/p1741201924377989, @m4olivei found issues with BigPipe and declarative shadow DOM not being compatible.

The additional specificity of the ID selector (like we did in offcanvas) is a great solution that would be relatively easy to apply if we can't get the web component method working.

I understand most of that. I think I need to get XB back up and running to test this stuff out.

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

OK, this should be good to go!

Couple notes:

• I aligned all the content to Olivero's grid as was specified in Figma. I thought this was going to be much more of a pain, but I got it working with the help of subgrid!
• I also fixed an issue with Gin applying a negative horizontal margin to Olivero's tabs. I only noticed this when I was like, "Why the hell isn't the tabs aligning to the grid?"
• I fixed (I think) the issue with in the schema that will break XB... this assumes it's the example data for the image
• The image in the MR is mine (I sent this over to @kwiseman during the FLDC sprint. Did I mention that Florida DrupalCamp is the best Drupalcamp in the world?
• I did not test this within XB (my XB is broke AF). But I tested this manually with the following code in a copied over page.html.twig.

You'll want to place this immediately below the {{ page.hero }} around line 96

{{ include('drupal_cms_olivero:hero', {
            title: 'This is my title',
            summary: 'This is my long summary. This is my long summary. This is my long summary.',
            image: {
              src: '/themes/contrib/drupal_cms_olivero/components/hero/images/example-hero.jpg',
              alt: 'The Grand Canyon during the Winter with snow',
              width: 2000,
              height: 1500,
            },
            link_text_1: 'Learn more',
            link_url_1: 'http://www.herchel.com',
            link_text_2: 'Click me',
            link_url_2: 'http://www.drupal.org',
          }, with_context = false) }}

volkswagenchick → credited mherchel → .

working on this

Thanks AmyJune!

mherchel → created an issue.

@grifstuf was working on this at FLDC (see the first comment). I know he had code written, but not sure why it's not pushed.

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

Thank you!

mherchel → created an issue. See original summary → .

Should we call this something generic like Accordion

makes sense to me! Updating title

mherchel → created an issue.

OK. Just pushed final fixes. This should be good to go. Note I refactored the CSS and markup significantly, so setting to NR.

mherchel → created an issue.

mherchel → created an issue.

OK! I think we're good. I updated the example image with the goodest boy on the planet, which is a photo that I take and am happy to relinquish rights, assign whatever GPL/CC licenses needed.

Another situation that I ran into is if an image component has an invalid examples.

For now I'm going to delete the file in the MR. Before merging we need to figure out where its being used, and make sure nothing breaks.

I would not think this should be a blocker. but if the LOE isn't much, it would definitely be a big deal

mherchel → created an issue.

This will be a great one to work on at this weekend's Florida DrupalCamp.

Just left a review with a number of changes! Thank you!

Fixed!

Thanks for volunteering and doing this @hot_sauce

mherchel → created an issue. See original summary → .

How do you imagine this would work? Which SDC props should accept this?

I would assume that it would be the latter (icon equivalent of $ref: json-schema-definitions://experience_builder.module/image).

This would then provide a form element similar to the gif in the issue summary. This would then (I think) return a SVG which could then be inserted into an icon component.

Does that make sense? I'm not too particular on the implementation. There might be lots of gotchas, but I would like to make sure the editorial experience is great (with the dedicated icon picker), and that the DX is great by allowing an easy way to specify the icon widget from within the SDC's component.yml file.

I'm not 100% convinced we should be handling image styles on this level.

This solves a real need. I'm not sure what other alternatives we are.

I believe we should have an easy way to create responsive images in the context of a component so that I don't have to go to the Drupal UI to create these.

💯 Agree! But the same goes with image styles. I personally use responsive images only on larger images (700px wide). The majority of images are smaller.

If we implement this, then we'd want to always retrieve the source image and applying the image style would happen after running this filter.

I'm not sure I understand this. Why do we need the source image?

mherchel → created an issue.

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

The root of the problem is that Olivero's CSS specificity on that page title block is greater then core's visually-hidden CSS class, so the Olivero width is taking priority.

I added a utility CSS file with an !important flag on the width property.

This should be good to go, but leaving it at NR since I added the most recent code.

Do you have another suggestion for how to fix this?

Ahh didn't know this. Will take a look.

Left a review. The CSS specific to the frontpage is unneeded.

I'm fine with either ECA or preprocess.

Looks perfect to me!

catch → credited mherchel → .