Gainesville, FL, US
Account created on 21 February 2007, over 17 years ago
  • Senior Front-end Developer at LullabotΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡Έ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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ changed the visibility of the branch 3446078-olivero-content-shift to hidden.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

imo it should be possible to ignore desktop somehow.

Yeah, that makes sense. Should be doable!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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).

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US
πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Looks perfect!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Ignore this.

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Removing more references to Drupal 8. 

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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. 

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

@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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ changed the visibility of the branch 3441137-bigpipe-injecting-local to hidden.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ™Œ πŸ™Œ πŸ™Œ

Thanks @catch!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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?

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

@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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Updating title.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

I agree that we should not prioritize this

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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?

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ’―πŸ’―πŸ’―

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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...

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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?

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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).

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

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

πŸ‡ΊπŸ‡Έ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

This is so beautiful. Everyone has done (and is doing) amazing work!

  • I pulled down the diff and applied it against a fresh 11.x install.
  • I tested out the functionality and it works as expected, and as intended.
  • It looks absolutely so beautiful.
  • I was able to effectively navigate around via keyboard
  • I tested in forced colors mode, and although there are are some minor issues, the menu is perceivable, operable, understandable, and robust.
  • All threads appear to be resolved.
  • I did not fully review the code, although it appears that it has been thoroughly reviewed. I will spend more time on this after it enters beta

RTBC! πŸŽ‰

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

should this be turned into a generic class to ap

I say no. If native CSS had the equivalent of Sass mixins, it'd be easy to do, but that's down the horizon. The current selector that we're using is .views-tabs__action-list-button:not(.views-tabs--secondary *), which is somewhat complex, and would be more of a pain in the butt to replicate in templates/preprocess.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Done! Yeah, I was copying and pasting an outline from google docs (and then converting it into HTML to paste here), and I missed the last line.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

I feel where we're at a point where the highest levels of the DA and the core team are now aware of this and are willing to dedicate resources (money). I'm about to be in the process of putting together a rough proposal and budget to take to the board of directors to make this happen.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

πŸ‘‹ Hi everyone. Its good to see this discussion happening here. We've been having similar discussions with the Drupal Association and various core committers and stakeholders.
Some of my thoughts (that I'm fairly confident are shared across the group)

  • Great documentation is a real blocker for people getting started in Drupal.
  • Our competitors make this easy.
  • We agree that we need to have a file based documentation system that is easy to keep updated.
  • Currently there is no one empowered to make tough decisions within Drupal's documentation
  • The correct documentation is hard to find alongside the "noise" of old or invalid documentation

The outline below clearly lays out the problems, goals, and proposed solutions and more.

Problems:

  • People do not have a positive first experience when coming to use Drupal’s documentation. Why?
    • The information is β€˜generally’ there, but hard to find
    • Information is very inconsistent quality and un-clear if it’s up-to-date or not
    • Outside of the user guide, there’s no β€œstart here, go here next”
      • The user guide is very β€œbuild your first blog with Drupal”. After that it’s not clear β€œgo here next”.
    • There’s a lot of good information on api.drupal.org but its hard to use
    • Google points a lot of people toward Drupal 7 documentation
  • Documentation is a pain to keep up to date. Why?
    • Use of HTML
    • Review process is weird
  • Invalid documentation sits around for years
    • No one is empowered to delete it
  • Documentation working group is inactive, probably needs new members who are currently engaged in Drupal community
    • It's hard to write world-class documentation by committee. Especially without good governance.
  • Updating the user guide (which is in Git) can be a pain in the butt for Neil (this according to Joe)
    • Additionally it’s difficult for anyone else to be able to make changes and preview those changes since it’s difficult to build the guide locally. It would be easier for translation teams to preview changes for example.

Goals:

  • Lower burden of maintenance for documentation system
  • Make it easier to keep documentation updated
  • Separate docs based on Drupal major version
  • Tender loving care
  • Quick turnaround to documentation changes
  • Differentiate between β€œofficially maintained” and β€œcommunity maintained” docs in order to limit scope

Solution being proposed:

  • Overhaul documentation to use build documentation to use an out-of-the-box open source industry-standard platform (e.g. ReadTheDocs)
  • Hire or contract a person to oversee the documentation system. This person would be part of the documentation working group, and would be empowered to make difficult decisions.
  • Split documentation between β€œofficial” and β€œcommunity”. Hired person would have
πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Note that I placed this in both the Views UI module's CSS, and Claro's CSS (which overrides the Views UI CSS)

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

I think I found a really good workaround for this.

  1. Checks if JS is disabled. If it is, nothing changes
  2. Hide any of the buttons that are not in their final destination (as placed by JS).
  3. The buttons are hidden by an animation that sets display: none in its initial keyframe, and the animation is set to fill backwards.
  4. The animation is delayed by 5s, and when it runs (which is set to 0.1s), it will remove the display: none.

So the net result is that the unstyled buttons will be invisible for 5s max. So, if JS is broken they'll pop in at that time.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Thats a good question :)

When I intentionally break that JS, the buttons will no longer show, which is a regression.

Setting back to NW. I'm pretty confident I can find a way around this. Plus it might make sense to move this out of Claro and into the Views CSS, as the JS that moves it around lives there.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

I might try to get a proof of concept up (in a separate MR in case it's a dead-end) for the block build alter idea.

I'd appreciate it. I don't have too much contrib time available, and this is a bit outside of my wheelhouse.

Local actions are also shown in Olivero - especially on entity pages like nodes, so I think this can equally be an issue there no?

We don't place the primary admin actions block (as Claro does).

Setting back to Needs Work for now, so you (or anyone else) can work on the block build alter thing.

Thank you!

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

Isn't this an issue for all themes? Could we add preview HTML to local actions generically?

I'm not sure. I'm assuming the template naming convention (big-pipe-interface-preview--block--claro-local-actions.html.twig)is based on Claro's core/themes/claro/config/optional/block.block.claro_local_actions.yml config. That naming convention would obviously change based on the theme.

And, if I recall correctly, that was the only template suggestion that was shown within Twig debug (I had to set an early JS breakpoint to see this).

Right now, I'm trying double-check and get the local actions block to be pulled in by BigPipe, and it's not happening. I'm not quite sure how Drupal determines if it's going to immediately render the block or let BigPipe handle it, but I'm having issues right now checking the template naming suggestions.

That all being said, IMO this is an easy win, and fixes something that I've definitely noticed in client sites in the past. Claro obviously powers most Drupal admin pages (and Gin is a subtheme of Claro), so this would positively affect the vast majority of users.

Setting back to RTBC to get committers' attention. Feel free to kick back if needed.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

MR created. Here's a video of the block being added now. Note how space is reserved and the button appears without shifting any other content.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

MR should be ready to go! Here's a video (taken with throttling set to "Fast 3g") for comparison.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US

mherchel β†’ changed the visibility of the branch 3441124-views-ui-action to active.

Production build 0.69.0 2024