🇺🇸United States @mherchel

mherchel → created an issue.

The 3250234-back-to-top-mikes-sdc-branch above is ready for review. I added a JS fallback for older browsers and fixed some linting.

Note the scroll progress only works in Chromium as progressive enhancement. I don't want to add a scroll event, which has the potential to slow down the browser.

@ehsann_95 I see you're still putting in a bit of effort on the other branch. You're welcome to continue to do so, but there's still quite of work to be done. You're welcome to review the new MR, which should be a bit closer to complete.

I created a new branch using SDC and CSS only (using scroll animations).

Right now the scroll stuff only works on Chromium. The main functionality works on all browsers.

That being said, I think we might still want to use Intersection Observer so that the show/hide works also works on all browsers.

Left some general comments. We should also move this to a single directory component. It's a perfect use case.

🙌 🙌

Yay! Thanks @finnsky and @nod_

Is the polyfill still needed here, it looks like popover has support in all supported browsers now?

As of now, it's needed. But I'd guess that it won't be needed by the time this finally lands.

• Only supported by Safari 17.5. Right now, Drupal supports the last two major versions of Safari. But... Safari 18 is going to come out this fall
• Only supported by Firefox 128. However, Drupal supports the latest release of Firefox ESR, which is currently at 115. But, the next ESR release is due this September, and will support it.

So... TLDR, we need it now, but likely won't need it in a few months as newer versions of the browsers get released.

Fixed!

• Data attribute prefixed with drupal
• When I set the attribute, I set the value to 'true'. Setting it to an empty string didn't feel right when I was checking the value of it. I'd have to either 1) check for the empty string 2) check if not undefined 3) use triple bangs (!!!)... all of which seemed kinda weird for me. Either way, I think it's good now!

I don't have the bandwidth to lead, but I'd love to help out on this!

conduct some competitive research/analysis

I'd love to help out with this specific task.

My thought is that the off-canvas only shows the title and date. So, IMO there's no reason to keep things super short, as long as it's easily scannable and not too drawn out.

Anyway, feel free to disagree, but I wrote an initial draft halfway down the document. I split up the events by continent, and didn't omit any (which would likely piss people off).

IMO it doesn't seem too long or windy. Thoughts?

mherchel → created an issue.

Maybe we should add a message under the "Status update" page if the file isn't local?

Looks good!

MR ready for review. Playing around with it, I ended up at 500ms.

mherchel → changed the visibility of the branch 3461284-prevent-simultaneous-openclose to active.

mherchel → changed the visibility of the branch 3461284-prevent-simultaneous-openclose to hidden.

mherchel → created an issue.

Also fixed and released a version for D7 https://www.drupal.org/project/quicklink/releases/7.x-1.1 →

Added to the new 2.0.3 release. Thank you @HeikkiY!

Tests passing. Committing.

We have a note in the docs to download the library locally for performance reasons. See https://www.drupal.org/docs/contributed-modules/quicklink/installing-the... →

Thanks for the heads up. I'll remove this. We also probably need to fix some tests for thsi.

I wonder if this issue should be credited and closed now that it's a stable part of the theme system?

Makes sense to me. Closing and crediting, since SDC is now a part of core and stable (in 10.3). Thank you everyone!

Dries already announced this functionality during the Driesnote. Should this just be closed?

per #20, I'm RTBCing this!

Comment #14 is getting me all excited! 😅😍😆

+1 on adding @pdureau as maintainer. His help during development was invaluable.

He is not the only one that should be added IMHO, cross linking 📌 Add e0ipso as a co-maintainer of core theme system with focus on SDC Active

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

The Chromium issue above is slightly different. It deals with setting display: contents directly on the <button>, which is not what we're doing.

From what I see, we're only applying it in one place on .admin-toolbar__scroll-wrapper

I'll investigate a bit more.

+1 RTBC. Not setting the width of the element that displace monitors until it's at the appropriate width makes sense. Good solution!

mherchel → changed the visibility of the branch 3443576-mobile-version-of to hidden.

@finnsky Good call! Fixed and ready for review.

mherchel → changed the visibility of the branch 3446078-eliminate-olivero-jank to hidden.

mherchel → changed the visibility of the branch 3446078-olivero-content-shift to hidden.

mherchel → created an issue.

imo it should be possible to ignore desktop somehow.

Yeah, that makes sense. Should be doable!

Or move it on core level out of module.

Yeah, this is my thought. In the MR above, I abstracted it to be on the core level, and then make use of it in both navigation and olivero (to show that it can be used in multiple places).

Looks perfect!

Ignore this.

I've worked on this. Got it figured out. Please reivew

mherchel → created an issue.

What charsets do we support? Supporting more will obviously result in a larger filesize.

Refreshing issue statuses

Removing more references to Drupal 8. 

Removing references to Drupal 8, since that's so old. Also updating the text to point to the new(ish) change record on how to customize placeholder content. 

Looks good! Tests are passing and space is now being reserved for the button. Thank you @catch!

@catch mentioned in https://drupal.slack.com/archives/C7AB68LJV/p1714544076932459?thread_ts=...

One last commit on the MR after that - used   so the link actually shows up (then gets wider) instead of being invisible, I have no idea if that is better or worse, feel free to revert the last commit if you don't like it but would re-RTBC otherwise.

We do need the invisible class there. Otherwise the placeholder will be visible as a very narrow button (it'll be visible because it has a background color), and before it's replaced. IMO its better for the actions to just pop in.

I reverted that last commit. Going to do a bit of testing, and then should be ready for a code review.

mherchel → changed the visibility of the branch 3441137-bigpipe-injecting-local to hidden.

The latest changes 1) correct the regression, and 2) fixes the original issue.

🙌 🙌 🙌

Thanks @catch!

Crap. This causes a regression on /admin/structure though. The local actions BigPipe placeholder loads... but since there isn't any local actions, BigPipe simply removes the span, causing the layout to shift up.

Is there anything we can do about this? Not sure if we can limit it to routes or some other way?

Yeah, verified that it's working correctly now. The containing elements are both 48px height before and after BigPipe gets to work.

Note you could also yank out that <li> if you want.

Doesn't appear to be working (Screenshot below).

To test this out

1. Disable JS/CSS aggregation
2. Disable caching on /admin/config/development/settings
3. Go to a "Manage field" page such as admin/structure/types/manage/article/fields. Note this is the only page where I consistently see this loaded by BigPipe
4. Set a JavaScript breakpoint at the very beginning of drupal.js
5. Hopefully the local actions are not loaded. Use your inspector to find the span. Ideally this should be populated with placeholder data.

If we didn't add the ul, would just the link be enough to avoid layout shift? That would keep it closer to stark markup then.

I think so. I just experimented and I don't think the UL is adding any extra margin in Claro. I'll double check when the code is ready, but right now I don't think we need it.

Getting close! Visually, this looks great, with the minor exception that there should be a gap between the logo and text.

The main issue is that there are two <a> tags, which would be a bit confusing for people using assistive technology. All of the text and image should be wrapped within one <a> tag, and elements should be added in that as needed.

The use of CSS grid on branding text is basic (although it works), when I mentioned this in #44, I was thinking to align the items within the <a> tag. I probably should have been a bit more specific.

@catch thoughts on committing the template solution, and then creating a followup to implement it with hook_block_build_local_actions_block_alter()?

Although this only solves the problem with the local tasks block with the Claro theme, I'd argue that is is a very significant reduction of jank for 99% of users.

Updating title.

could be good to have in a blog post somewhere :)

Thanks! Yeah, I have one somewhat thought out in my head about reducing jank, and this would be a part of it. Just need to find time!

@lauriii commented at https://drupal.slack.com/archives/C7AB68LJV/p1714402683585049?thread_ts=...

I agree that we should not prioritize this

Are you sure about this? My thought process is that if we make it useable without JS, it will likely negatively affect the complexity of the code, with much more work needed to be done to make this work 100%.

My thought is that we should do a best effort on this and maybe make sure the hyperlinks work?

I also recall @lauriii stating that within the admin UI (in general), it's not necessary to be 100% workable without JS.

As an aside, maybe it's worthwhile putting together a formal policy on functionality being able to work without JS. Thoughts on all of this?

Opened followup: 📌 Navigation: Discuss/implement splitting variable font Active

mherchel → created an issue.

Thanks for working on this! Looks to be working as expected now. In addition to the administrator/anonymous screenshots below, I also created an authenticated user that doesn't have access to use the navigation bar and made sure the font wasn't loading for them.

Looks great!

About font split. Do you think it has to be addressed here? Or might be moved to a lower priority follow up?

Definitely a followup. Probably needs a bit of discussion and maybe even testing.

Thanks! Putting back to RTBC per #18 since all that changed were code comments.

We should be looking at potentially splitting the font up to reduce size

💯💯💯

Just pushed some changes with the comments.

As far as creating a CSS Variable, I'd like to push that over to 📌 Investigate possibility of creating API, CSS class, reusable animation to help eliminate page jank Active . There we can also abstract the @keyframes animation. But my thought is that we'd probably want to create a new library, and we'd need to decide where that lives (within Claro, system, etc)... which to me seems out of scope of what we're doing here.

Let me know if that's alright. If not, I can create the variable and put it in the views_ui.admin.css file.

mherchel → created an issue.

mherchel → created an issue.

Maybe for the js-hide class we could use the media query to make sure we hide things asap.

We already got that committed in 🐛 Table filter creates jank (layout shift) on page load Fixed . See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...

Yay! We're in core! Now let's get in 10.3.0!

I noticed that there are some comments missing, and other minor things that could be improved.

So, I don't see any beta blockers listed in 🌱 [PLAN] New Toolbar Roadmap: Path to Beta & Stable Active . Is this the only blocker? If so, can you elaborate or should we go though again?

What actionable steps are needed to get this into 10.3.0?

Adding 📌 Investigate possibility of creating API, CSS class, reusable animation to help eliminate page jank Active

mherchel → created an issue.

I'm willing to create a class or do whatever it takes to get this done, but I do think doing so will be premature optimization at this point. I'm not 100% positive I know a place where we'd be able to re-use it.

I started a slack thread in the #contribute channel at https://drupal.slack.com/archives/C1BMUQ9U6/p1714158882541689

Change record created at https://www.drupal.org/node/3443793 →

I would argue that it doesn't need a CR (although if you disagree I'm happy to do it). This was a legitimate issue that pre-existed both the announcements and navigation modules. The same issue would affect anything using the off-canvas dialog, or any other component that uses drupal.displace() and the node preview container.

RTL issue with node preview container is fixed. Good to go!

Fixes in #41 are correct, but do not take into account RTL because it's using logical properties with Drupal displace (which is not language direction aware).

Fix incoming in a couple min.

Confirmed that it is BigPipe. Everyone now owes me $10.

This isn't a beta-blocker, so let's fix this after it gets into core (which should be very soon).

Ten bucks says this is caused by BigPipe injecting the data. Should be able to be fixed with https://www.drupal.org/node/3338948 → with a similar technique as 🐛 BigPipe injecting Local Actions block creates large janky layout shift in Claro Needs work

mherchel → created an issue.

mherchel → created an issue.

Adding 🐛 Mobile version of Navigation should have focus trap Active

mherchel → created an issue.