🇺🇸United States @mherchel

CR looks

to me!

Looking at the 11.x, the current top bar is is an <aside> with an aria-labelledby attribute that points to its visually-hidden heading, with the text "Administrative top bar"

From my point of view, I don't see any reason to change this (please correct me if I'm wrong).

There are currently two <nav> elements in the sidebar, which we will consolidate to one.

If all this looks correct, I'll update the IS. I also feel like we might need to have a discussion in Slack to make sure we're on the same page. The requirements of this issue keep changing, and we need to lock it down to make progress.

I hate to bikeshed this.

From @rkoller in #56:

For the sidebar i would use a navigation landmark and label it with primary, that way it would be crystal clear that this is the main navigation for the admin ui.

I worry that on the front-end of the site, the primary label would be confused with the primary navigation of the site. My thought is it could be named administrative toolbar Thoughts?

mherchel → made their first commit to this issue’s fork.

Yeah, I was also wondering about the 'nesting-rules': { edition: '2021' },. Per the docs it defaults the the behavior of previous PostCSS nesting (which is not actually what browsers implemented).

The new rules (2024-02) cause the browser to accurately reproduce specificity by wrapping parent selectors in an :is(). This mimics the actual behavior of browsers (which basically wrap parent selectors in an :is()).

I really doubt the 2024-02 rules will cause any regressions, but it's impossible to be 100% sure unless we have some type of robust visual regression testing going on, which we don't.

My suggestion is to stick with the 2021 rules for now. Early on after 11.2, lets update to the 2024-02 rules, which will more accurately reproduce the specificity that browser do. Then we'll have several months to find any potential regressions.

After that, we should really see what's holding us back from shipping native nesting. A quick look at https://caniuse.com/css-nesting, shows that our supported browsers support it.

OK. Should be good!

Fixing now

I like the naming in #35!

Should there be a reverse relationship too? An example of this would be to limit people to put accordion_items only into accordion_group components.

Good catch!

The issue was that the top bar was rendering, even if it didn't have content, which pushed down the popover.

I added a check for content within the twig file. Should be good to go. Thank you!

Yay! Test is passing. Note that I don't test out displace's functionality (that gets tested in its respective tests), I just test that the attribute is added properly, which is what the JS in this module does.

Yeah the current tests passed when I re-ran them.

Just pushed a simple nightwatch test. Lets see if this passes (I didn't test it locally 😬🤞)

Ready for review! Screenshot attached :D

mherchel → changed the visibility of the branch 3526266-navigation-top-bar to hidden.

mherchel → created an issue.

Fix attached.

mherchel → changed the visibility of the branch 3526180-regression-drupal.displace-not to active.

mherchel → changed the visibility of the branch 3526180-regression-drupal.displace-not to hidden.

mherchel → created an issue.

Somehow I missed this.

Tables can also be added via CKEditor, which is why we had it as a global library. You can see at https://git.drupalcode.org/project/drupal/-/blob/11.2.x/core/themes/oliv..., we're also targeting tables that appear in .text-content (which is formatted text).

wim leers → credited mherchel → .

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

wim leers → credited mherchel → .

andyg5000 → credited mherchel → .

Heck yes! This totally fixes the issue for me. I'm attaching two videos:

1. Part 1: The video showing the issue existing
2. Part 2: The video showing patch installed, and the issue solved

Still leaving a NR, since we need a code review and I'm not super qualified for this level of PHP. But confirmed to fix the issue!

evidently the test case isn't as simple as I initially thought. I've run into this a lot, but need to see what the differentiator is that causes it.

Updating the steps to reproduce

Slack discussion at https://drupal.slack.com/archives/C079NQPQUEN/p1746998199858839

In it, backend framework manager @larowlan states (in regards to @deviantintegral approach in the issue summary)

that approach looks fine to me, ideally with the cache clearer injected

Bumping this to Major (note it may need to be critical). Most new themes will have SDCs, and this will affect anyone who install new themes via the UI.

smustgrave → credited mherchel → .

Any chance this can be backported to 10.x? It'd be nice to use this features and have themes be compatible with both D10 and D11. My understanding is that XB hopes to support D10 also.

This works perfectly!

I ran the command in ddev, and worked as expected.

The theme appeared under the appearance route as expected.

I played around with it and tried to break it, and it worked great!

Running this thing through its paces 🤞

➕💯➕💯➕💯➕💯

mherchel → created an issue. See original summary → .

Crap. Forgot to actually change the field!

Bumping to major.

With Drupal's focus on site templates, this becomes a blocker for that functionality.

yep!

g4mbini → credited mherchel → .

Just pushed a 2.0.5 release with this fix (and fixes tests!)

Crediting Matt Glaman. I owe him so many beers!

mherchel → created an issue. See original summary → .

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!