Gainesville, FL, US
Account created on 21 February 2007, over 17 years ago
  • Senior Front-end Developer at Agileanaย  โ€ฆ
#

Merge Requests

More

Recent comments

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Thanks for the response.

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

I will look at those other issues.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

Should be GTG!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

future proofing versions

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Updating polyfill info

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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)
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ’ฏ

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Updating IS

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Fix looks good and works!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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?

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Fixed!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

@weekbeforenext reviewed this in Slack.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

 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? 

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ changed the visibility of the branch 3464065-support-view-transitions to hidden.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

@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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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 .

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

๐Ÿ™Œ ๐Ÿ™Œ

Yay! Thanks @finnsky and @nod_

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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!
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

conduct some competitive research/analysis

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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?

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Looks good!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ changed the visibility of the branch 3461284-prevent-simultaneous-openclose to active.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ changed the visibility of the branch 3461284-prevent-simultaneous-openclose to hidden.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Tests passing. Committing.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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... โ†’

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

per #20, I'm RTBCing this!

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

Comment #14 is getting me all excited! ๐Ÿ˜…๐Ÿ˜๐Ÿ˜†

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

+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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ created an issue.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ changed the visibility of the branch 3443576-mobile-version-of to hidden.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

@finnsky Good call! Fixed and ready for review.

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US
๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

mherchel โ†’ changed the visibility of the branch 3446078-eliminate-olivero-jank to hidden.

Production build 0.71.5 2024