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

Merge Requests

Recent comments

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

second best camp

NO CREDIT FOR THIS MAN!!!!

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

Brought back up to date and should be good to go!

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

@divya.sejekan great catch! However, I'm moving this to a followup, since this is a minor UX issue, and shouldn't block this core MVP issue.

Followup is at πŸ› Content shift when expanding filters accordion Active . I'm going to bring this back up to date, and set back to RTBC since no major changes.

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

Please credit @divya.sejekan who found this in the parent issue.

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

mherchel β†’ created an issue.

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

Let's not resort to !important unless it's absolutely necessary.

A better fix would be to override the exact same selector (

  .claro-details[open]>.claro-details__summary:focus::before,

) within the forced-colors media query. You could add this on line 292.

Note that the details.pcss.css file needs quite a bit of refactoring at some point. It doesn't make use of modern CSS, or nesting like many other Claro stylesheets do. But in the meantime, let's keep the stylesheet consistent and keep the same patterns.

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

Discussed within Accessibility office hours February 15th (today) with several folks including A11y maintainers Mike Gifford and Ben Mullins.

This is a very low priority, and we're struggling to figure out the problem that this is solving. A potential problem, that's not in the IS, is that maybe the named anchor link will get styled unnecessarily, but I haven't seen this.

According to Ben Mullins, we need evidence that this is a problem, which we do not have. Closing and if anyone can post evidence that this is a real problem, please re-open.

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

I don't see any need to update the list.

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

yeah, this is on me (sorry!). I've been a bit snowed under with both work and personal stuff.

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

This is likely related to 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work . I wonder if within region--sidebar.html.twig, if we change the {% if content %} to {% if content.0 %}, if that'd be a good enough fix. It seems to solve the problem on my local.

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

I'm pretty sure the root of the region output issue appears to be 🌱 [meta] Themes improperly check renderable arrays when determining visibility Needs work , although there are likely ways to work around this.

I'm still not able to reproduce the wrapping issue though. I even set up book, and got the extra element in there, but no wrapping happened.

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

Also Happy Valentines Day everyone! β™₯️

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

The current patch needs a different approach. The pager needs to wrap if 1) narrow viewport or 2) user zooms in where the items would otherwise be off the screen. In addition, @GΓ‘bor Hojtsy is right that the pager needs to accommodate a sidebar that can potentially have more content than the main region.

@GΓ‘bor Hojtsy refers to two issues

(1) the pager wraps when there is a sidebar block
(2) there looks like whitespace produced from block templates that the main page template thinks is sidebar content

However, I'm not sure if we can resolve the first issue because the pager does not know how much content is in the sidebar region. If it overflows, it has to wrap. If the pager were in its own block, we could move it to a different full-width region, but that's not the case.

The main issue here is that we need to make sure that the region markup does not output when content is not present.

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

I think that in itself is a bug.

Yeah, I agree with that. If you disable that block, does it fix the pager issue?

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

I can't reproduce this with out of the box Drupal. When I look at your site, I do see the issue, but the markup is changed a bit (not sure what's doing it). There's also a sidebar block that's displaying empty markup. That might be related.

Not sure how to troubleshoot this, because I can't reproduce. Ping me in slack and we can figure out time to troubleshoot.

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

We use this permission on https://fldrupal.camp. Our use case is that we let users with a "speaker" role submit session nodes. But when submitting this node, we want them to stay within the current frontend theme. So to do this, we removed the permission for "View the administration theme" for this role.

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

According to https://developer.mozilla.org/en-US/docs/Web/CSS/:link,

The :link CSS pseudo-class represents an element that has not yet been visited. It matches every unvisited or element that has an href attribute.

My worry on this patch is twofold:

1. The changes will break visited links. There's mention of this in #29, but we definitely want link styling to still apply to visited links.
2. The :link pseudo-class creates extra specificity that might break other styling. Let's use :where() to remove this specificity. Something along the lines of a:where([href]) as the selector.

Thanks for all the work on this everybody. Sorry to set this back to NW.

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

Just seeing this now. This is per the design (see https://www.figma.com/file/x5zBLbvoW1jsvyAOt4Gp9I/Olivero-Theme---Public...)

Closing as 'works as designed'. I appreciate all the work though.

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

Removing unnecessary browser info

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

done! The code sample was actually there. Just forgot to remove the placeholder!

Change record is at https://www.drupal.org/node/3406417 β†’ , since the syntax above doesn't work with CR's, I guess.

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

mherchel β†’ created an issue.

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

Here's a quick UI review. Note that this doesn't apply to 10.2.x or 11.x, but it does apply to 10.1.x, which is how I left my review.

1. This link doesn't work. It links to the same page. It's especially confusing because when you hover the mouse over it, the tooltip says "open link in a new tab"

2. The modal's form doesn't have a "submit" or "create" button.

3. How does the option for "Allow the user to create download links" work? I didn't see how to do it.

4. The help text of the "Entity Links" checkbox doesn't tell the site builder anything helpful.

Maybe something like "Provides an autocomplete interface for creating links to Drupal entities that will auto-update when the entity's path changes."

5. If you create a new suggester via the modal, the page reloads and the original suggester is still selected. The newly created one should be selected if possible.

Overall this is looking amazing! Thanks for the hard work on it!

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

@larowlan

Just resolved all the threads. Thanks for your feedback in there. Let me know what's next.

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

I think y'all will like it

😍😍😍

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

Did this make it to the core meeting yet? If so when is that scheduled?

Also, what information needs to go in the change record? I don't see any at https://www.drupal.org/list-changes/coding_standards β†’ Does the info need to be extensive, or does it just need to note that CSS coding standards have changed?

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

Why not .js-hide.js-hide like we do usually when we need to increase specificity?

No specific reason. Attribute has the same specificity as a class, so πŸ€·β€β™‚οΈ. Not sure if there's a preference.

Shouldn't we add the extra specificity here too?

Nope. We don't need it. The extra specificity is only for when JS is enabled, and therefore the current (previous) ruleset works file. We only need to take into account for situations where JavaScript is enabled but the .js class is not yet present.

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

@mherchel is there an additional step? I'm on Version 119.0.6045.159 (Official Build) (arm64) and applying the patch I'm not noticing any difference on the page load

Ha! You're just like me and skipped over part of the testing steps (I do this all the time). Here's the part that you missed 🀣🀣🀣

Note that the at-media feature is not supported in the latest version of Chrome (119). It is supported in the next version of Chrome (120), Safari 17, and Firefox. So, if you're testing on Chrome, please verify that you're on 120 or later (you can use Chrome Canary if you'd like).

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

Thanks for all the work on this! @ckrina and I briefly look at this on a call, and then I dove deeper into it today and left a couple comments.

The main points are

  • We cannot use pixels as units. This is an accessibility requirement to pass the core accessibility gate. I understand that themes might set different root font sizes, but if we determine that's problematic (I personally think that's a problem for that theme), we can switch to use EMs at a different point.
  • We should emulate the reset at https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/misc/dialog.... This is well tested and should use revert instead of the initial property.
  • Talking with @ckrina yesterday, she was wanting to make more generic reset that could also be used on the toolbar. Assuming we go that route we should do something like this:
    .drupal-admin-styles:is(*, #reset) *:where(:not(svg, svg *)) {
      all: revert;
      box-sizing: border-box;
      -webkit-font-smoothing: antialiased;
      line-height: 1.4;
    
      &::after,
      &::before {
        all: revert;
        box-sizing: border-box;
        -webkit-font-smoothing: antialiased;
      }
    }
    

    in the example above the :is(*, #reset) adds extra specificity equal to the strongest selector in the :is() pseudo-class, which in this case is is an ID. So the selector will match the .drupal-admin-styles class, but have the specificity of an ID.

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

Thanks for working on this! I tested this out and ran into a number of issues:

1. I'm unable to delete entire directories.

I declared starterkit: true in Olivero and tried to delete the tests directory by adding

delete:
  - 'tests'
delete:
  - 'tests/'
delete:
  - '/tests'

Or other permutations, and wasn't able to get it to succeed.

2. If nothing found under no_edit or no_rename, no error is thrown

I expect an error to be returned if the process doesn't find any files that it can operate against

3. Wildcards / globs don't work

I tried passing in

no_edit:
  - '**/js/*.js'

and it still edits the files.

3. No longer any need for starterkit: true in the YML file

The presence of a themeName.starterkit.yml is indication enough

4. Nice to have: debug mode

It'd be super nice if this process could have a debug mode to help out theme developers.

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

I haven't yet tested this out, but I'm concerned about using the html.js method for styling without JavaScript. This can lead to a flash of unstyled content and perception that the content is jumping around on page load.

A better way to do this would be using https://developer.mozilla.org/en-US/docs/Web/CSS/@media/scripting. Note that it won't be available on Chrome for 2 more versions (120) but it is available in FF and Safari. By the time this toolbar becomes stable, this feature will be stable.

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

Here's a test case where I create a dependency on the olivero/feed library to try to force the SDC to load CSS at the end.

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

Test case patch with a simple component within Olivero. Note that within the SDC, I don't declare any library dependencies.

This is the source order of the CSS.

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

Found a bug:

if you manually input 0df into the field, it thinks it works. The JS looks good, etc. But when you navigate to the homescreen the color is all black

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

The theme generator tool is a one-shot CLI script run by a developer, so can it just depend on drupal/core-dev?

IMO this is a bad idea. It creates bad developer experience for front-end developers who are not familiar with modern PHP / Composer (which there are many).

An example use case is a front-end developer who gets up and running using lando or DDEV, by running an automated script. Installing additional composer dependencies seems onerous after already having to run a command to generate the theme.

πŸ‡ΊπŸ‡ΈUnited States mherchel Gainesville, FL, US
  • Looks like issues have been addressed.
  • Tests exist and pass.
  • Code looks good to me.
  • Seems to work great
  • Properly escapes data

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

Looks like tests not passing CS

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

This is the reason newspapers are printed in columns and well-designed web pages don't let their text go to the full width of the window.

Agree that for reading stories and normal text, 80 should be the max (I actually prefer less). However reading normal text is very different than reading code.

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

formatting :) 

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

Adding clean_unique_id (see  https://www.drupal.org/node/3363551 β†’ )

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

adding sdc_display

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

Updating warning text.

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

Should flyouts have a focus trap?

No. The point of a focus trap is to disable anything from gaining focus while not visible (if it's behind a flyout or modal). We might need to add logic to have the flyouts close on blur (focusout), but that can be outside of this.

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

I don't think it should be a hard blocker

+πŸ’―

Perfect is the enemy of good. That can come later IMO

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

This is now available at https://lb.cm/drupal-navigation with help from @q0rban and @rabbitlair

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

Adding credits.

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

This is now available at https://lb.cm/drupal-navigation with help from @q0rban and @rabbitlair

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

Thanks everyone!

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

Just merged πŸ“Œ Generate menu items using plugins RTBC .

Had discussions that fit into this issue with @deviantintegral and @tedbow at https://drupal.slack.com/archives/C7AB68LJV/p1695052420501349.

Both of them are confused on where the data is coming from.

From @tedbow

I am little confused about the purpose of this issue vs using the menu system. It sort of uses the menu system, as in NavigationAdmin uses a menu but NavigationUser does not. Eventually will these all use menus? If so couldn’t you have just as much control over the markup by just having specific templates for these menu rather than just introducing the new NavigationSection concept
does 'base hook' => 'menu', mean that all the theme process stuff will still apply to these navigation sections also? (edited)

from @deviantintegral

I was also a little confused about this, but assumed I'd just missed prior context. The big thing I see is that I could see modules adding items to navigation sections that aren't menus, but some other UI widget. Like I think environment indicator is a good example of that? But I also think until we start wanting to create releases there isn't much harm in merging the MR, given that it's less hardcoded than the current code.

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

Had discussions with @deviantintegral and @tedbow at https://drupal.slack.com/archives/C7AB68LJV/p1695052420501349.

Both of them are confused on where the data is coming from.

From @tedbow

I am little confused about the purpose of this issue vs using the menu system. It sort of uses the menu system, as in NavigationAdmin uses a menu but NavigationUser does not. Eventually will these all use menus? If so couldn’t you have just as much control over the markup by just having specific templates for these menu rather than just introducing the new NavigationSection concept
does 'base hook' => 'menu', mean that all the theme process stuff will still apply to these navigation sections also? (edited)

from @deviantintegral

I was also a little confused about this, but assumed I'd just missed prior context. The big thing I see is that I could see modules adding items to navigation sections that aren't menus, but some other UI widget. Like I think environment indicator is a good example of that? But I also think until we start wanting to create releases there isn't much harm in merging the MR, given that it's less hardcoded than the current code.

For now I'm committing this (so it can unblock some work on the templates and CSS). I'm adding this additional comments and info to 🌱 [PLAN] Use Menus to generate the links in the sidebar Active , where we can figure out how we want to get this data.

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

I got this working at https://tugboat-navigation-xqru9se3afaasli8m0kv1mtt624s6ven.tugboatqa.com/

Ping me in slack for the admin username/pw.

I also pinged @q0rban to create a short URL for this at lb.cm/drupal-navigation

Setting to NW till that gets done.

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

I think we definitely want to have the logo customizable. I also think it needs to go through the image style process so it looks and loads quickly.

Whatever route/path we put this on, we also need to think of other future settings like colors, dark mode, etc that hopefully will be coming down the line. My initial thought is somewhere in system settings.

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

Thanks for the reviews @deviantintegral and @tedbow!

What's the next step here? Should these be followups, or does this issue still need work?

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

The templates are in good shape now. There's a number of @todo's that we can get to later.

Still needs PHP and functionality reviews.

Note that when tried to test this, I ran into an error

The website encountered an unexpected error. Please try again later.

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "plugin.manager.navigation_section_manager". Did you mean this: "plugin.manager.navigation_plugin_manager"? in Drupal\Component\DependencyInjection\Container->get() (line 157 of core/lib/Drupal/Component/DependencyInjection/Container.php).

I resolved this by ) checking out a previous version of the code (7db614c10cb472347875141e19d239cbe2df5624), 2) uninstalling the module, 3) checking out the latest version of the code, and 4) reinstalling the module.

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

Currently working on the templates here to ensure strings are translated, markup and aria attributes are correct.

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

We might add other toolbars (like a top bar for the save button and other actions)

I would argue that this is the admin toolbar. Other toolbars will be something else (the top bar for save, etc would be "Action toolbar")

Looking at the admin_toolbar css, it looks like they use .toolbar, .toolbar-tray-horizontal, .toolbar-menu. I think we can use .admin-toolbar.

https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/css/admin.to...

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

What's the next step in this process? From what I gather, the https://www.drupal.org/project/coding_standards β†’ process text has changed since the last time I looked at it, so we're in a weird place.

The issue should have a change record.

It looks like we need a change record. Should that be on this project, or Drupal core?

If the committee agrees to the change the issue is tagged β€œNeeds documentation updates” and is announced on https://www.drupal.org/about/core β†’ with a consideration period of 14 days. If there are concerns or conversation is ongoing, the issue consultation time may be extended. If the committee does not agree to the change the status may be set to "Won't fix" or to "Needs work" with an explanatory comment.

I think this already happened. It was announced at https://www.drupal.org/about/core/blog/coding-standards-proposals-for-fi... β†’

If the issue affects core it is discussed at the next core committer meeting.

Has this happened yet? Is it on the agenda for the next core committer meeting?

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

For A, I think we should use "toolbar". I don't think we should use "sidebar" as that's a typical region name in many themes. We also shouldn't use "navigation" because this isn't the primary navigation of the site.

Maybe we can use "admin toolbar"?

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

Flyouts aren't working either due to the overflow clip.

Setting this to NW now because it still needs a bit of work.

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

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

Looking at it visually, when I switch between branches there is a difference in spacing. Gif below

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

There's a lot going on here. It's really hard to review.

  1. It looks like the markup is also being 100% refactored (+390. - 385). Why are we doing this?
  2. We're also updating PostCSS Preset ENV here. Why? Is this keeping up with core?
  3. We're refactoring checkOverflow() within the JavaScript.

Can we split this up into smaller chunks? Right now, I see the CSS which is this issue, checkOverflow, PostCSS Preset ENV, as well as all of the markup changes.

For now I'll review the CSS here, but it's really hard for me to go through all of the markup changes when there are almost 400 removals and 400 additions just with that.

Still leaving at NR so we can review the CSS.

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

Can you bring this up to date with 1.x now that πŸ“Œ Refactor JavaScript Fixed is merged?

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

Can you bring this up to date with 1.x now that [3386509] is merged?

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

Committed dbba902c

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

This is ready for review. Note I'd like to get this in ASAP, as any further JS work will conflict with this.

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

Von, Thank you so much for being part of the community. The work you did bringing DrupalCon back (after skipping 2 years because of Covid) was amazing, in addition to other things such as Discover Drupal. You're dedicated, hardworking, and are a great person. Your next stop is lucky to get you!

Production build https://api.contrib.social 0.61.6-2-g546bc20