🇺🇸United States @michaellander

michaellander → created an issue.

Added as maintainer! Appreciate your interest!

michaellander → created an issue.

If you are on 10.1 and above, try the latest 2.0.x release. If you are on 10.3 and above, try the latest 2.1.x release or test the 3.0 alpha. Let me know if you are still seeing the issue!

Moved this into 3.x.. I'm not opposed to backporting it, but my concern is how significant of a change in approach it is from 2.x versions. The new 3.x alpha should be all the latest with this, and then some plans for a few additional improvements elsewhere.

@JeroenT I'm going to move the CI stuff into a separate issue and close this. Thanks!

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

We've been using the 'Overview' approach patch from ✨ Allow modules to hook into top of content/footer sections of new core navigation Active as a stop gap because users were getting so frustrated they couldn't access the 2nd level when a third level item was added. In our particular example it was a combination of the commerce → and scheduler → that was making it impossible to access the commerce dashboard.

In this particular use case I don't believe commerce should define a submenu link to 'Products' because as far as it's aware, it has no other children. However, we also can't put the onus on 'scheduler' because any number of modules could extend commerce in ways that they feel warrant adding children to the 'Products' menu item and then you have an issue with many modules determining how to keep from breaking the 2nd level 'Products' link.

I'll be honest, so long as we treat the dynamic between the 1st and 2nd level different from the 2nd and 3rd level, where the 1st level is linked at the top of 2nd level, but 2nd level is not accessible at all if 3rd level exists, I believe we'll always have this confusion. I think this also includes hiding/showing behavior as related to \Drupal\system\Controller\SystemController. Perhaps an additional property could be added for Menu links to define an 'Override' title when children exist, but again that seems like something that should be consistent between the 1st and 2nd levels as well.

It's for these reasons that I think the split button is most elegant, but I'd even take the 'Overview' solution over no solution.

I went ahead and created ✨ Allow modules to ALSO hook into top of footer section of new core navigation Active so that we could come back to it when timing is more appropriate.

michaellander → created an issue.

This feels like a duplicate of ✨ Add "All" or overview links to parent links Active , which feels like a duplicate of 🐛 Second level menu items can't be reached if they have children Active .

Added. Thanks for your contribution!

Looks like this may be related to or a duplicate of 📌 Parent menu items with URLs are not linked to, do we make these available? Closed: outdated

I think part of the challenge we are finding while using navigation every day, is a module might add an admin link to the navigation, without any children, with the expectation that a user goes there to configure it. Other modules might extend that modules functionality and add sub pages. So a page that may have originally stood on it's own inadvertently becomes the overview for other modules. I definitely get the idea around eliminating the generic overview link list pages, but in an open ecosystem, it's definitely likely that a sole page could later become a parent.

With progress over at ✨ Integrate with Workspaces Active . We may be able to start to adapt our approach to work more similarly, realizing things are still changing. I'll try to find time in the next few weeks.

Nice work! For the switchers, we could probably just use Drupal\Component\Utility\Html::getClass() on the environment label to generate a wrapper class name, and use that class to switch the variable values in a given context. Could always wrap the name generation in a utility function/method?

:root {
  --environment-indicator-bg-color: blue;
}
.environment-indicator--production {
  --environment-indicator-bg-color: red;
}
.environment-indicator--develop {
  --environment-indicator-bg-color: green;
}

Thoughts?

Thanks for creating the issue! For anyone interested, I put some of this in ✨ Support for core navigation experimental module Needs work .

Specifically:
https://git.drupalcode.org/issue/environment_indicator-3457688/-/blob/3457688-support-for-core/environment_indicator.module#L144

However, I stopped short of adding it for non-active environments. Was looking for more feedback!

This should be working better now.

I had this consistently working and now with a fresh install it's not... needs more work.

This should give us a path forward for modules like ImageAPI Optimize AVIF → to run alongside this module without needing glue modules like ImageAPI Optimize AVIF & WebP → .

If we can get some more eyes on ✨ Attempt to remove ImageStyleDownloadController override completely Needs review , I believe it'll make this update much easier overall. I think it allows us to keep this in 2.1, and should give us a path forward for modules like ImageAPI Optimize AVIF → to run alongside this module without needing glue modules like ImageAPI Optimize WebP and AVIF →

This version seems to fix issues around overriding the Drupal\image\Controller\ImageStyleDownloadController completely and is much more elegant. We will need to test to make sure everything is flushing correctly when the image is replaced.

Also going to test ✨ Attempt to remove ImageStyleDownloadController override completely Needs review , which would really simplify things. In which case maybe we just move to 3.x if it works.

michaellander → created an issue.

@danielwirz, am wanting to include 📌 Automated Drupal 11 compatibility fixes for imageapi_optimize_webp Needs review in a 2.1 release so we can confidently support Drupal 11. If you can help test the patch in that issue on 10.3 and 11, that'd be great.

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

I've added the necessary core_version_requirement and tested in Drupal 11, and all looks good.

michaellander → changed the visibility of the branch project-update-bot-only to hidden.

michaellander → changed the visibility of the branch 3431022-automated-drupal-11 to hidden.

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

I've created a 2.1.x branch for 10.3 and above. I need to test the compatibility of the D11 fixes with 10.3, found here:
https://www.drupal.org/project/imageapi_optimize_webp/issues/3448709 📌 Automated Drupal 11 compatibility fixes for imageapi_optimize_webp Needs review

And then I'll create a 2.1.0 release. We should probably upgrade 2.0.x to not allow anything above 10.2.

Hiding patch and moving to branch.

michaellander → changed the visibility of the branch 3453941-declaration-of-drupalimageapioptimizewebpcontrollerimagestyledownloadcontrollerdeliver to hidden.

Looks good, thank you!

@ckrina - These look awesome! Appreciate the feedback. Definitely would be great to be able to use the same templates as core.

@bronzehedwick - I think it's because some of my css depends on the toolbar module being enabled(unintentionally).

I'm also using CSS variables for part of it, but in some areas I am using inline styles, which is the existing approach. I think it would be nice to align the approach to be the same (All css variables, or all inline styles). I've just been waiting for feedback and maybe things to catch up a bit!

The module has already set precedent for handling other toolbars without submodules. When Navigation is the defacto standard, do you intend to move the ToolbarHandler behavior into a submodule as well? I think it's a rather confusing experience that if you are using the older toolbar, you just install Environment Indicator core, but if using the new Drupal core Navigation, then you need the submodule. We could always mimc the ToolbarHandler approach in making it responsible for checking if it's applicable(and just move the lazy rendered in there). The submodule just feels unnecessary.

With Navigation being in core(since 10.3), I don't think it makes sense to require a submodule. And any hooks required simply just won't fire if navigation doesn't exist.

@daddison, good question. I believe I wrote it with layout_builder_restrictions_by_region enabled, but I don't believe it is required. It should just be respecting whatever the scope of permissions(section vs region) you have configured. The test scenario will be that the region won't even highlight for you to drop a block in unless it's allowed, instead of waiting for you to drop the block and then get the pop-up warning. If you are testing locally, and have cache turned off, along with a lot of dev tools enabled, it may be kind of slow to validate. Though we've been using it in production on a handful of sites and thus far the performance lag has been negligible.

This is really cool! I'd be curious if there's areas it would make sense to start and define UI/design conventions around its use. I think the fade in/out makes sense in most cases because it's subtle, but maybe areas like Same Page Preview being able to slide the edit form over to fade in the preview, etc etc..

I'm still a bit conflicted, in that I think we should just allow modules to extend on the layout builder usage, and also allow users to add blocks to the navigation footer. However, I realize there's some hesitancy here, so we still should consider giving some means for modules to extend these areas.

michaellander → created an issue.

Looking into it more, we'd probably want to remove the dependency for gin_toolbar and core's toolbar and instead would want to conditionally work based on what modules exist on the given site. Definitely needs more work and maybe some guidance from maintainers.

It looks like the code is not correctly handling when the 'toolbar' module is turned off, so this needs more work.

Updated the css to be a bit closer to concepts:

We started using layout builder in mega menu's awhile ago with great success, and seeing layout builder implemented in the new navigation builder successfully is awesome. I'd love to see it used here. We could have blocks for 'Entity Operations', 'Form Actions', etc. With the left toolbar acting more global, and the top bar holding more contextual info, we could take advantage of the existing Context system for some of the block visibility handling. This approach makes it far easier for other modules to extend and override the top bar without having to override templates everywhere.

I'd be happy to build that in similar fashion to the left hand toolbar if it would be helpful.

Confirming @Balu Ertl's fix, both modules just need the info.yaml update. I went through the environment switcher UI to create new environment entities and everything was fine. Active environment settings in settings.php also was fine. No issues in the logs.

I pushed up another version:

I did an initial attempt at the CSS, but my CSS nor UI design is my strength. I did use an approach with CSS variables, and I did not create a new Handler class. If we do want to go the Handler route, maybe we take advantage of service tagging in the early returns to avoid hardcoding every potential Handler check.

I also have left out the 'configure' link, but not opposed to adding that back either. I could probably just use feedback and/or design inspiration before going further.

I've made decent progress on this(see blue button on bottom left):

@ckrina, in the version you have, there are no child links to actually do the environment switching. Did you have another idea of where that belongs or is my approach on the right track(with some cleanup)?

It's using a lazy builder similar to many other blocks in navigation so that we can respect which environment links to show per the users permissions. I should be able to have a fairly complete version pushed back into the fork by tomorrow.

Is it worth including some examples to pressure test on the extreme end of low-code customization? For example a heading where you can adjust font-style, font-weight, heading level, margins(top, bottom, left, right), padding(top, bottom, left, right), display, position(top, bottom, left, right), text properties(color, align, decoration, etc), height, width, line-height, etc etc...

This would be both useful from performance/storage perspective, but also in some UI decisions.

I like that as an interim solve, and would allow us to pivot in the future if core makes the blocks that are allowed override-able. I'd be happy to push this forward while you're gone.

Ha @trackleft2, I just came here to mention the same thing. I was able to make a quick proof of concept, but I just prepended a render array in environment_indicator_preprocess_navigation in $variables['content']['content']. I think your solution is better, but I wonder if in the interim we could just prepend the rendered block in preprocess(this would make it always first, right after the logo).

Additionally, I was curious if inline styles is still the preferred approach, or if we could consider using CSS variables?

  $css_variable = '--env-indicator-fg-color: ' . $active_environment->get('fg_color') . ';';
  $css_variable .= '--env-indicator-bg-color: ' . $active_environment->get('bg_color') . ';';
  $variables['#attached']['html_head'][] = [
    [
      '#tag' => 'style',
      '#value' => ":root { $css_variable }",
    ],
    'env-indicator-css-variables',
  ];

michaellander → created an issue.

I pushed some styling fixes that seem to be working pretty well on desktop at least..

Front end is not my expertise, but I felt like rather than completely overriding the gin template, just using the `form_actions` variable already defined intop-bar--gin.html.twig would make it easier to support. This branch fixes functionality issues but still needs styling fixes.

michaellander → created an issue.

Thanks for the patch. We'll likely need to create a 2.1 and make a requirement for 10.3 and above.

What I've found is the approach with this module and some of the others in the space are a bit dated(and require overriding a core controller which created this issue to begin with. I'm working on a potential new alternative that I think would be foundationally stronger, but in the meantime we'll keep supporting this.

So it might be significantly more than this module would want to try and handle, but I was curious if you could instead render layout builder in the admin theme, and then use an iframe for the front end preview. Obviously this creates a new set of challenges, but the trade off would be to keep styles from bleeding over between frontend/backend. Obviously this would be a significant departure from what the module is doing currently, but at least there would be less fighting with non-api theme overrides.

What about providing an alternative to fields for many props in SDC's(and elsewhere) that really have no need to be full fledged fields(or at least wrapped in a single json field)? Fields are great for content, but when I think of low-code style modifiers(alignment, min width, max width, heading weights, levels, etc, etc) having a field for each is very heavy handed and slow to build out for front enders/builders, and doesn't provide a great admin interface without a lot of work. Perhaps letting fields focus on content and allowing display/style attributes to be simplified would alleviate some of the bloat.

michaellander → created an issue.

I thought this was interesting: WebP fallback image → .. Their argument is that we aren't gaining the full benefits of compression with modules like ImageAPI Optimize Webp → because we aren't compressing from the source file. My original rationale was to avoid running images through image style processors multiple times(once as jpg, once as webp, for example), though that wouldn't matter so much if it's only running it through the image style as it's being requested for the first time. I think it's also more and more likely that people will favor AVIF with a WebP fallback, and almost never want to keep the original file format if JPG, PNG, etc..

Perhaps finding a way where we would no longer need to do the derivative handling at the imageapi_optimize level, and let optimize just worry about optimization.

I haven't looked at this stuff in awhile and will try and get caught back up.

Another useful resource: https://component.gallery/design-systems/

I really think this would be a nice addition to this module, and I would be happy to make it something that is an optional setting if we think it's too big of a change to introduce by default. Would love some feedback from a maintainer. Thank you!

Hi @bjc2265, I've tested this again and it's still working for me. Have you seen any error messages in console or drupal logs? I will say that if you have a lot of debugging turned on, or caching turned off, it may be a little slow(as with all things layout builder). Right now the default behavior is to prevent dropping anywhere until validation is done. I could always change it to allow dropping anywhere until validation is finished, which would fall back to the currently build-in validation. With normal cache settings enabled, it has been pretty seamless for me but I want to make sure that's true for others.

Going to need to see a resume and some references. ;)

I've added you, thanks for jumping in!

@weseze, can you clarify the part that this patch is missing? Is it the idea that cropping/scaling a JPG image prior to conversion, automatically introduces compression, even if set to 100% quality?

My hands might be tied from this modules perspective, as we are piggybacking pipelines which naturally happens after image styles are applied. If I had the time, I'd create a module(or better yet, core functionality) whose sole purpose is to generate derivatives and then allow support for multiple formats(AVIF for example) so that ImageAPI optimize can go back to just optimizing and not the derivative piece.

Really appreciate this @pdureau! We will try to get these in shortly.

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