Fix issues when creating custom menu in Regions

Hello coaston, I have Fixed this issue. created MR & added Screenshot as well.
please review & retested it.

Upon reviewing the Merge Request (MR) and your proposed changes, I observed that the styles have been placed in main.css and specifically target .menu--custom-menu. While this does address the issue for custom menus, it seems like the problem also persists in other menu blocks across the theme, such as the Tools menu and Footer menu, indicating a more global styling adjustment might be beneficial.

1. Spacing Issue:
- Affected Areas: Tools menu, custom menus, and footer menu exhibit identical spacing issues wherein the menu items are conjoined without adequate spacing.
- Visual Impact: This absence of spacing impacts visual separation and readability of menu items.
2. Coloring Issue:
- The previously reported color issue (black text on a black background) seems to be non-replicable in the current theme version. However, it is crucial to ensure that any introduced styles do not inadvertently reintroduce this issue.

Proposed Solution:

In light of the above, a global styling adjustment is proposed that ensures consistent spacing across all menu items within the theme, excluding the last item to prevent undesired end spacing:

/* Global styling for all menu blocks */
nav {
  &.block-menu.navigation li {
    &:not(:last-child) {
      margin-right: 10px;
    }
  }
}

I have created a new branch and applied these changes.

@tguerineau and @shiv_yadav, thank you for your help on the project and for providing the patches.

I have a couple of items to bring up.

@tguerineau For this patch: https://git.drupalcode.org/project/rigel/-/merge_requests/45/diffs. Please provide the compiled main.css as well.

Please refer to the README.md https://git.drupalcode.org/project/rigel/-/blob/6.x/README.md?ref_type=heads for setting up the environment for compiling the CSS.

Once you commit the main.css, I'll test the patch out.

Hi @baldwinlouie , thank you for your review.

Regarding the Provided Patch:
I’ve noticed the requirement for the compiled main.css. While preparing to compile, an unrelated issue was identified which prohibited successful compilation of the SCSS files:

Issue Identified:

In the _formelements.scss file, there’s a reference to a variable $button-radio-ripple that seems to be undefined or possibly mistyped:
animation: $button-radio-ripple 0.5s linear;

Given the existence of $button-radio-ripple-size, it seems plausible that this might be the intended variable, but we need collective insights to confirm this.

My Approach:

To proceed and provide a compiled main.css, I opted to adjust the variable reference to $button-radio-ripple-size. This enabled the compilation to proceed, however, this introduces a change that is not directly related to the initially addressed menu spacing issue.
- The MR Includes both the original fix for the menu spacing and the adjustment to enable SCSS compilation.

Seeking Guidance on Issue Management:

Given that these fixes are bundled, I wanted to seek your advice on the best approach to manage this within the issue queue:

Option 1: Keep both fixes within the current issue and merge request, with detailed notes explaining the inclusion of the SCSS compilation fix.
Option 2: Create a new issue for the SCSS compilation error and submit a separate merge request, even though the current MR includes both fixes.

I’ll proceed to update the MR with both fixes (the compiled main.css and the compilation error). Your feedback, testing and insights on both issues will be valuable. Thank you.

@tguerineau Thank you for the insight and investigation. This has been really helpful. I dug a bit further, and found that the complication error was due to this issue:

https://www.drupal.org/project/rigel/issues/3386284 📌 Comply with Drupal coding standards (3) (Stylelint) Fixed , and specifically, the change in this screenshot:

We are ok with Option 1: fixing the issue in the same MR. I think the correct fix is to revert the following:

Changing from:

    &:checked + label::after {
      transform: scale(1);
      animation: $button-radio-ripple 0.5s linear;
    }

to

    &:checked + label::after {
      transform: scale(1);
      animation: button-radio-ripple 0.5s linear;
    }

since button-radio-ripple actually refers to

@keyframes button-radio-ripple {
  0% {
    box-shadow: 0 0 0 1px rgba($d-text-primary-color, 0.25);
  }
  50% {
    box-shadow: 0 0 0 $button-radio-ripple-size rgba($d-text-primary-color, 0.25);
  }
  100% {
    box-shadow: 0 0 0 $button-radio-ripple-size rgba($d-text-primary-color, 0);
  }
}

in _variables.scss.

Let me know your thoughts.

Hi @baldwinlouie, thank you for your review.

SCSS Compilation Fix:

I have made the suggested adjustment:

&:checked + label::after {
  transform: scale(1);
  animation: button-radio-ripple 0.5s linear;
}

This alteration aligns with the defined @keyframes button-radio-ripple.

The MR is now updated with this change.

@tguerineau Thank you for the patch. This fixes the initial issue. Also thank you for fixing the compilation error as well!

Here is what the menu looks like on my local environment.

@yas, please sanity check this patch. If it looks good to you, let's get this committed.

@baldwinlouie

Thank you for your review.

@tguerineau

Thank you for your contribution. I'm about to merge your MRs however before I merge them, I have a couple of requests for you:

1. Can you please arrange the SCSS for MR !44?
2. Can you please create a new MR by integrating your two MR w/ SCSS and compiled main.css in between MR !44 and !45?

Thanks!

@yas. @tguerineau and I have decided that https://git.drupalcode.org/project/rigel/-/merge_requests/44 is not as generic and shouldn't be merged in. @tguerineau provided a better solution in https://git.drupalcode.org/project/rigel/-/merge_requests/45

Hope that helps.

@baldwiinlouie

Thank you for your comment. In that case,

We close the strong>MR !44 w/o merging it.
We merge MR !45 only.

Can you please confirm it?

Thanks

@yas, that is correct.

@baldwinlouie

Thank you for your confirmation. I'll merge the patch to 5.x and 6.x, and close this issue as Fixed.

yas → closed merge request !44

Automatically closed - issue fixed for 2 weeks with no activity.