🇺🇸United States @mherchel

smustgrave → credited mherchel → .

smustgrave → credited mherchel → .

One is display the link in an anchor tag. Here the trim makes sense because you are not breaking the link.

I would argue that although the trim works within an anchor tag, it should definitely not be the default. On practically all sites I work on I have to modify this default (on each and every link field on each display mode). If we were implementing this new, there's no way we would choose to truncate by default (although it should still be an option)

Thanks for the response.

I re-checked 11.1.x and the problem is still occuring :(

I will look at those other issues.

I'm continuously running into this. Any progress? I'd like to help but not sure where to start.

volkswagenchick → credited mherchel → .

volkswagenchick → credited mherchel → .

Just pushed a commit with regions and a disclaimer. Regions are necessary.

Should be GTG!

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

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

mherchel → created an issue.

future proofing versions

Updating polyfill info

This fix will also affect many of the input types that should not be affected (color, number, etc).

We want to ensure there's no regressions.

This problem only seems to occur when the input is nested within a table, which happens when multiple images are allowed in the field settings.

I'd recommend only targeting the input types that need to go full width, and scoping them to a table.

Just seeing this now. Thank you for all the work, but this was an intentional decision made during the implementation phase (not sure if it's documented though).

Closing as won't fix.

I'm experiencing this issue as well.

@jenna.tollerson note that that this is more likely to occur when you're logged in, as Drupal will be running attachBehaviors more.

The MR fixes the issue for me and seems to be great. Attaching the patch here so I can reference it via composer patches.

I wonder if it'd be worth it to add a warning (or info message) on the Status Report page in the meantime.

rajeevk → credited mherchel → .

gábor hojtsy → credited mherchel → .

I want to give a hearty +100 to this. It's something that I've been thinking about for a while.

Updating IS with next steps

1. Find out who needs to give sign off
2. Get sign off on the initial idea
3. Figure out technical implantation (this will require DA staff as well as core changes)

Great work on this. Such an improvement.

A couple points on images:

1. All image styles should convert to the WebP format.

This provides as great resolution and transparency as PNG, but for a tenth of the file size. The current site downloads almost 5MB of images! We should be able to get this down to 500kb, even with this image heavy visual appearance.

2. Do not lazy load images "above the fold"

The hero image is lazy-loaded. This will make the image appear to load a bit slower, and also increase the site's LCP (largest contentful paint) which is bad. Fortunately this is easily fixed in the field formatter.

3. Utilize responsive images, and do not load images as background images in CSS.

Loading background images via inline styles in CSS makes it difficult to use image styles, and also makes it difficult to use responsive images. You can work around this by loading a regular <img> tag in the HTML and then absolutely positioning it and use object-fit

I'm not sure what to do here or where this is going. The designs at https://www.figma.com/design/x5zBLbvoW1jsvyAOt4Gp9I/Olivero-Theme---Publ... do not have the "Read more". Just because it was there before doesn't mean we still need to do it or that it's proper UX.

From @catch in #27

However if a design decision was taken to do this in Olivero I don't think the figma is sufficient to explain that, there should be a 'why' somewhere to go with the pictures.

Not sure there's any insight other than, "it's not needed".

Also this is another example of node display elements being controllable only via twig templates and not in manage display, layout builder, or content type settings. If these were implemented as an extra field and exposed manage fields/layout builder there'd be no special cases template variable at all.

💯

Discussed during meeting today.

Per @m4olivei:

It looks like this adds an alter hook that allows the form to be able to placed within the navigation module. We need to do two things:

• Does this solve our issue and allow us to place the form?
• Will this new hook work for other modules that want to do similar?

@m4olivei will work on this.

Updating IS

I don't know TBH. My preferred local setup was MAMP with SQLite. But since that stopped working for me, I switched to DDEV.

At one point I tried a git bisect, but somehow got confused and screwed that up.

Fix looks good and works!

Language links are working now thanks to @phenaproxima's help

OK. It's looking alright. The language dropdown works, but the links don't.

Maybe we should postpone this until that is done. We're in no hurry here as far as I know.

The CSS rule can be simplified like @finnsky suggested in the MR

See my comment in #6. The CSS rules cannot be simplified (unless all browsers support style queries). The reason is that all buttons get assigned a class with that includes [class*=toolbar-button--icon--], even if there is no SVG icon.

The style query works fine. But doesn't yet support FF and Safari (Safari's not a big deal because I don't believe people use that with Windows High Contrast).

So to be clear, I'm going to remove the long list of selectors, create (and postpone a followup), and then add a @todo?

can it use same class selector? [class*="toolbar-button--icon"]

This is a good thought, but unfortunately the "toolbar-button--icon--xxxx classes are added even if an icon doesn't exist. If we apply the styles to the items that do not have icons, then the helper text (the two letters) doesn't show.

Markup with an icon

Markup WITHOUT an icon

per @ckrina: Let's use a basic gradient from https://drupal.widencollective.com/portals/gfvztttq/BrandPortal#:~:text=... for the image

This will likely be changed at a later time.

I've talked with @phenaproxima and @timplunkett about this and will take this on. Self assigning.

I'll plan to get some work on this by mid-next week.

This looks amazing. I love how grid can clean up the code so well. We had those funky margins in there because we had to support IE11 🤮

Anyway, I have one nice-to-have that I think we should get in here:

Instead of using --layout-threecol-grid: repeat(4, 1fr); to set up the grid, let's use minmax(0, 1fr) to set the maximum grid width to 1fr. Otherwise, if there's super wide content that doesn't fit, it will stretch the grid.

So, something like --layout-threecol-grid: repeat(4, 1fr); would become --layout-threecol-grid: repeat(4, minmax(0, 1fr));

Otherwise this is perfecto.

This looks good. Not doing the pre-rendering unless people ask for it.

Fixed!

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

Yay!

@weekbeforenext reviewed this in Slack.

mherchel → created an issue.

 This functionality is currently offered as a contributed module, but will eventually be built into Drupal core

It's in core now right? This should be removed, right? 

Fixed and verified the specificity. This should be RTBC as soon as the tests are passing.

Yeah, I like the path of using media queries. The CSS looks good, although we need to spend a bit of extra time to make sure the rulesets still apply since the specificity is changing (the media queries don't add specificity like the HTML classes did).

Tests are failing, but I'm not sure why.

Will there be a virtual/hybrid option? I can't be there in person, but I do have beer at home.

We'll allow it! BUT... you have to post a picture in front of something GovCony, with a beer!

Thanks for the review. We also have some failing tests for some reason.

For chrome, while scrolling the screen around the button a progress bar was shown and it is also interactive while scrolling the page but this feature is not available for firefox.

Yeah, the scroll indicator uses a browser feature that's not yet available to FF. My thought is that this is not necessary for the functionality, so I consider it progressive enhancement.

One thing I would like to point out which is that, after clicking the button for scrolling back to top and when the page is at the top the button does not disappear at its own, we have to again click back on the screen and then it disappears.

Yeah, this is an accessibility requirement (WCAG 2.4.7 Focus Visible), which states as long as a control as focus, it must remain visible. I wonder if we should shift focus to something else.

mherchel → created an issue.

mherchel → created an issue.

mherchel → changed the visibility of the branch 3464065-support-view-transitions to hidden.

@shweta__sharma You need to be more specific if you want the summary updated. Removing for now. If you add it back, please document what you information needs to be there

Yeah, we should really not be doing development until the design is done (and it looks like it is not done).

From #14

Thanks for all for your work here, but changing the color for the main button in the UI needs one of the maintainers approval. This actually needs some design work to test how it looks in other situations too, so moving back to active since we haven't done the design round yet.

Setting this to needs work because we need designs before we can properly write the code.

Yep. This was an intentional design decision to align to the left. There's an issue to create an option to center align at 📌 Olivero: Center align content (instead of left align) Needs work .

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.