Support for core navigation experimental module

Created on 27 June 2024, 2 months ago
Updated 9 August 2024, 29 days ago

Problem/Motivation

The environment indicator module currently integrates only with the admin toolbar in Drupal. I wonder if there are any plans Integrating the environment indicator module with the navigation module in core.

Proposed resolution

I have an idea in mind, but it doesn't include the environment name, and does not consider custom logos.

Remaining tasks

  1. Create Sub module with the environment_indicator_navigationmachine name
  2. Set the Module Name to "Environment Indicator Navigation"
  3. Set the sub module's minimum Drupal version to 11.
  4. Remove code from sub module that has been deprecated in Drupal versions before 11.
  5. Add Readme to sub module.
✨ Feature request
Status

Needs work

Version

4.0

Component

Code

Created by

πŸ‡¦πŸ‡·Argentina dagmar Argentina

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @dagmar
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    Hi @dagmar, thanks for your help with ideas to integrate the Navigation with other modules.

    On our designs explorations for the Navigation we already designed for the Environment indicator, in a way that can co-exist with Workspaces. Here are the designs we agreed on, where the initials of the environment are also used as an indicator. They are really close to your explorations, which is great :) Changing the logo background was discarded because the logo can be replaced and either take the whole space or not work well with the color chosen for one of the environments. Credits for this designs should go to @jponch and @anabarcelona.

    Either way, I've added this issue to the Navigation roadmap. Thanks!

  • πŸ‡¦πŸ‡·Argentina dagmar Argentina

    @ckrina thanks good to know :) So the navigation module will allow a color change and the environment indicator will have to interact with this new API? No CSS changes are required from this module if I understand this correctly? Any issue in core to check this?

  • πŸ‡¦πŸ‡·Argentina dagmar Argentina
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    So the navigation module will allow a color change and the environment indicator will have to interact with this new API?

    I have no idea about the implementation, but I bet nobody has thought about it yet. @plopesc and @m4olivei are the right people to answer this. (@m4olivei was who brought this up to my attention so I'm sure he can help)

  • First commit to issue fork.
  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    I might be wrong, but I think the functions in the navigation module disallowing all block types other than the ones defined by the navigation module itself are what is standing in the way here.

    Example https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig...

    Blocks might need to get a new property called navigation_safe or something similar.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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',
      ];
    
  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    @michaellander thanks, I think we should try to keep all that we can from the existing render array in the new block.

    I think we should focus on creating the new custom block and Force it into the list with something like

    
    /**
     * Implements hook_plugin_filter_block__layout_builder_alter().
     *
     * Curate the blocks available in the Layout Builder "Add Block" UI.
     */
    function environment_indicator_plugin_filter_block_alter(array &$definitions, array $extra) {
    
      if (($extra['section_storage'] ?? NULL) instanceof NavigationSectionStorage) {
        $custom_block_id = 'environment_indicator';
        $definitions[$custom_block_id] = [
          'class' => 'Drupal\environment_indicator\Plugin\Block\Navigation',
          'provider' => 'environment_indicator',
          'admin_label' => t('Environment Indicator'),
          'category' => t('Navigation'),
        ];
      }
    
    }
    
    /**
     * Implements hook_module_implements_alter().
     */
    function environment_indicator_module_implements_alter(&$implementations, $hook) {
      if ($hook === 'plugin_filter_block_alter') {
        // Move environment_indicator to the end of the list.
        $group = $implementations['environment_indicator'] ?? false;
        unset($implementations['environment_indicator']);
        $implementations['environment_indicator'] = $group;
      }
    }
    
  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    Updated the css to be a bit closer to concepts:

  • Merge request !53Resolve #3457688 "Support for core" β†’ (Open) created by michaellander
  • Status changed to Needs review about 2 months ago
  • Status changed to Needs work about 2 months ago
  • πŸ‡ΊπŸ‡ΈUnited States michaellander

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

  • First commit to issue fork.
  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    Great progress on this so far.

    I think we should move this functionality into a sub module

    Then we can add the dependency on the navigation module in the sub module's info file.

    We should also set the minimum Drupal Core requirement in the sub module's info file to 11

    We should also be able to drop any code that has been deprecated in previous versions of Drupal.

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    Here is a possible README for the sub module.

    CONTENTS OF THIS FILE
    ---------------------
    
     * Introduction
     * Requirements
     * Installation
     * Configuration
     * Maintainers
    
    INTRODUCTION
    ------------
    
    The Environment Indicator Navigation sub-module enhances the Environment Indicator module by adding a navigation block with lazy loading capabilities to display environment-specific links in the Drupal admin toolbar.
    
    This sub-module leverages the new Navigation module, which integrates seamlessly with Layout Builder to provide a user-friendly interface for managing navigation menus in Drupal.
    
     * For a full description of the module, visit the project page:
       https://www.drupal.org/project/environment_indicator
    
    REQUIREMENTS
    ------------
    
    This module requires the following modules:
     * Environment Indicator (environment_indicator)
     * Navigation (navigation)
    
    INSTALLATION
    ------------
    
     * Install as you would normally install a contributed Drupal module. Visit:
       https://www.drupal.org/docs/extending-drupal/installing-modules
    
     * Enable the Environment Indicator Navigation sub-module on the Extend page
       (`/admin/modules`) or using Drush:
       `drush en environment_indicator_navigation`
    
    CONFIGURATION
    -------------
    
     * To add a new navigation block, go to `/admin/config/user-interface/navigation-block`.
     * Configure the block settings as needed.
    
    MAINTAINERS
    -----------
    
     * Maintained by Michael Lander (mlander) and the Drupal community.
    
    
  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    @michaellander I've created a merge request with all of my suggested changes against the 4.x branch.

    https://git.drupalcode.org/issue/environment_indicator-3457688/-/merge_r...

    Note that my version creates a new sub module called `environment_indicator_navigation`

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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.

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    Navigation is marked experimental in core, which I also marked the environment_indicator_navigation submodule.
    IMHO, the submodule approach adds a layer of safety and better code organization by isolating experimental features and allowing for controlled testing and rollouts.

    By moving code into a submodule, we ensure that services are only registered when their corresponding module is enabled, keeping the system modular, organized, and performant.

    Additionally, functions in the .module file of the submodule are only executed as part of its lifecycle, provided the submodule is enabled.

    This method not only keeps the main module clean and stable but also allows for a flexible and maintainable approach to introducing experimental features to a Drupal site.

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

    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.

  • πŸ‡ΊπŸ‡ΈUnited States bronzehedwick

    Thanks for all the work on this!

    I applied the patch, and I'm not getting the chosen environment color in the side strip or the environment menu. I dumped the variables and it looks like they're still set to the defaults. Can confirm caches are rebuilt.

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    Whatever you guys decide is fine with me.

  • Merge request !54Resolve #3457688 "Sub module version" β†’ (Open) created by trackleft2
  • πŸ‡ͺπŸ‡ΈSpain ckrina Barcelona

    wow, this is going fast and I haven't been able to comment before. Thanks all!

    As mentioned, we're working on a design for this that will have similar styles as Workspaces (as it's a stable blocker to get Navigation into stable). I know the modules are different, but the UI is similar in a lost of ways. We're working on the styles for the "drawer" once opened, but we haven't finished that yet.

    I'm sharing the designs here so you can see the direction (not finished designs yet since we still need to change several things, like the buttons), but I'd recommend not merging this until we've been able to finish the designs.

    When we finish the designs for Workspaces, it'll be implemented on the Navigation itself and you'll be able to reuse part of the code without implementing it here.

    About the screenshots I'm seeing: I'd recommend trimming the text as we're doing for the icons in the Navigation instead of applying a gradient. See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/navig...

  • πŸ‡ΊπŸ‡ΈUnited States michaellander

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

  • πŸ‡ΊπŸ‡ΈUnited States trackleft2

    Created a new issue for converting inline CSS to CSS variables πŸ“Œ Update module to use CSS variables instead of adding inline CSS. Active

Production build 0.71.5 2024