Grimsby, ON
Account created on 30 December 2008, over 15 years ago
#

Merge Requests

More

Recent comments

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

What happens if the module is installed via drush instead of the UI?

It depends. The following produces the expected result:

$ ddev drush si -y minimal
$ ddev drush en -y sdc_cache_issue
$ ddev drush uli

However the following reproduces the bug noted here:

$ ddev drush si -y minimal
$ ddev drush uli
[use login link to view the site]
$ ddev drush en -y sdc_cache_issue

So it would appear that there is some core code managing the front-end theme cache when the module gets turned on, but not the admin theme, and so the admin theme cache as it concerns SDC assets is stale.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

@finnsky I agree, there is pretty clearly some caching issue with SDC module components at play. I've filed the following issue: πŸ› Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active . It includes a sandbox module that I threw together as a minimal working example of the issue: https://git.drupalcode.org/sandbox/m4olivei-3460588

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Recent changes look good.

I came across one weird thing with this MR that I can't replicate on the 11.x branch. Here are some steps to reproduce it:

  • ddev drush si -y standard
  • ddev drush uli
  • Login and enable navigation module via UI, eg. go to /admin/modules, enable Navigation module and Save

Expected behavior

Navigation looks and feels like its supposed to.

Actual result

If you clear cache, things do start working as expected. Not sure what is going on. Cannot reproduce on the 11.x branch, so seems like something here introduced it.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I read the issue summary, comment and the MR. There is a remaining task of another issue but it isn't clear if that needs to be completed first. Does it?

I don't believe so. It doesn't need to be completed first. The added functionality here addresses logos that are too big. However, just like it is on 11.x at present, a user can still upload a logo smaller than the recommended dimensions, and we need a fix so we don't break the layout, which is the subject of ✨ Center small navigation logo on collapsed state Needs work . It's not a requirement for this issue. I've updated the issue summary. @SKAUGHT or @plopesc can correct me if I'm wrong.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Tests were also added and are already present for this feature as part of πŸ› Custom Navigation logo is disconnected from new Layout template Fixed .

I think we're save to close this one. We already have test coverage in place for this, and it will grow as part of πŸ“Œ Create Image Styles for Nav Logo Active .

Closing as outddated.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

This may not be needed b/c πŸ“Œ Create Image Styles for Nav Logo Active adds functional tests for this feature. It's close to being committed. Lets postpone on that one.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I'm unable to reproduce this issue with the latest 11.x.

Following the steps to reproduce in the issue description, I don't get the error reported in #8, the Navigation blocks UI does render. Furthermore, I can click on Save successfully without any issues.

Closing as cannot reproduce.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Just a tiny thing that I noticed affecting some of the screen reader text.

Otherwise, changes look great to me! This is a really nice improvement by my read. Curious to see what the subsystem maintainers / folks with more familiarity with SDC will come back with. Nice work @finnsky

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Thanks @quietone. I've updated the change recode and issue summery per your suggestions.

Back to RTBC🀞.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I think we're all set on all threads. Tests now passing and ready for a re-review.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Hey there πŸ‘‹. I was attempting to use menu_link_attributes to test something on Drupal 11.x and found this issue. I don't believe there is an actual problem with the code in question on Drupal 11.x or any version.

First, it was not obvious to me how to reproduce the issue or trigger the code path in question. Here's what I found to work:

  • Checkout the latest Drupal core 11.x
  • Install Drupal w/ Umami demo (eg. drush si -y demo_umami
  • Clone the menu_link_attributes module
  • Edit the menu_link_attributes.info.yml to add || 11 to core_version_requirement
  • Enable content translation for menus. Navigate to Configuration > Region and language > Content language and translation. Enable "Custom menu link". Expand "Custom menu link", enable "Translatable" next to "Custom menu link". Enable the "Hide non translatable fields on translation forms". Click "Save configuration"
  • Create a custom menu link. Set some menu attributes.
  • Translate the custom menu link and click Save.

After all that, the last step, saving the translated custom menu link will trigger the code path pointed out. There are no errors or warnings here when I run the module in Ddrupal 11.x. This makes sense, b/c the code here, eg.

\Drupal::entityTypeManager()
      ->getStorage($menu_link->getEntityTypeId())

Will load an object of type \Drupal\menu_link_content\MenuLinkContentStorage, and that class implements in it's interface hierarchy \Drupal\Core\Entity\RevisionableStorageInterface, which has the legit loadRevision method.

I think the error you're getting is simply from static analysis. At runtime, there is no actual issue.

Marking as Needs Review. Suggesting to Close as works as designed, and then open a follow up to get to 11.x support.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Sad trombone. This is now failing Kernel tests with the following error message:

Drupal\KernelTests\Core\DependencyInjection\AutoconfigurationTest::testCoreServiceTags
Service 'navigation.event_subscriber' in core/modules/navigation/navigation.services.yml should not be tagged with 'event_subscriber'.
Failed asserting that 'event_subscriber' is not equal to 'event_subscriber'.

/var/www/html/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutoconfigurationTest.php:33

Appears to be a new check added in πŸ“Œ Enable service autoconfiguration for core event subscribers Fixed . I'll work on the fix.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

This is looking good. All remaining threads are resolved. I've just merged 11.x to the branch to freshen it up and see how the pipeline is. Will wait on results and mark RTBC if all is well.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Hey there πŸ‘‹. I understand the role, and agree to follow the statements in the linked document. I'm happy to serve in this way. Thanks!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I took a crack at breaking the standstill on the one issue. Lets see what we all think.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Can confirm that all comments have been addressed. Everything is working well from a functional perspective.

  • Installing the module from fresh sets the expected block config for the footer navigation region.
  • No visual regressions on the Help link placement or otherwise.
πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

This is looking good to me! Items in #43 have been addressed. Tests are passing and making good sense to me.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Addressed comments from @plopesc. Ready for review.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

+1 RTBC. I tested locally and it works as you might expect:

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I'm also unclear if I could have put it back to RTBC in this instance? Would be curious to learn process there too. Thanks!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated the issue summary as well as drafted a change record. First time drafting a change record, so let me know if I missed the mark in any way.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I'll take a crack at writing a change record here.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated the merge request to address the <h4> issue. See MR comment for details.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

This is looking great. RTBC for me.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated the issue description summarizing comments in favor of using the Megaphone.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Love the thought here. One thing I noticed is that the spacing in the footer has some regressions.

Will also go through a static review shortly.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I've merged 11.x and made the changes requested in #6. Tests are now passing.

Ready for review.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I've tested locally. It's looking good!

Marking RTBC.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Looks like @finnsky's feedback on cleaning up the SVG was addressed. Moving to Needs Review.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Opened an alternative approach in MR 7944 which simply returns an empty render array when shortcuts module is not enabled. This avoids the fatal error. Eventually when we can move this logic into shortcuts module, it won't be necesssary.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Closing this issue as outdated. This came before the big change in the admin of Navigation over to layout builder. Most of the changes in the navigation contib MR here don't apply anymore. We can open specific follow up issues on an as needed basis for the current state of the navigation module codebase.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Sounds good!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Ready for review. While here I also made the selector more explicit for the assertion.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

To summarize the back channel in Slack. Now that we've got a clean branch agasint core to work with, we're seeing a failed test. That failed test is failing on account of changes to the Twig template that are including the abbreviation for the Top Bar button. The twig template changed gets used by the top bar (you need to drush en -y navigation_top_bar to see it on a node view/edit page). Probably don't want this in that case.

Should clean that up. Then we might still need to adjust the test depending.

@skaught correctly pointed out that we'll probably also want to move that failed test (NavigationTopBarTest) from where it is. That's the subject of πŸ› Navigation Test names are disjointed. Active .

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ changed the visibility of the branch drupal-3424744-3424744-on-collapsed-sidebars-off-11.x to hidden.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ changed the visibility of the branch 3424744-on-collapsed-sidebars-off-11.x to hidden.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I moved @bronzehedwick's MR over to Drupal core. There were quite a few conflicts, so I'm really not sure if everything is OK there. Take a look see.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ changed the visibility of the branch 3424744-svg-generate to hidden.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Thanks @adwivedi008!

The navigation module moved to core rencently in ✨ Add the new Navigation to core as an Experimental module Fixed , so we need to move this MR to be against Drupal core.

I've gone ahead and done that. Unfortunately there was no easy git way to move the commits since they were against two different repos.

Marking this as Needs Review. For me it's looking good!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

@fjgarlin correct, it was moved into core in ✨ Add the new Navigation to core as an Experimental module Fixed as an experimental module. And indeed, eslint is all good in that context.

Moving this issue back to the contrib module, although note that the contrib space is more or less abandoned for all intents and purposes. The core merge is still fresh, so the contib module is still not completely cleaned up and clear to the that effect. All that is to say, the navigation module contrib space might not be the best place to work on the eslint issue anymore?

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I fired up an MR thinking this would be easy. I spent a good half hour just finding where that border is defined that is obstructing the tooltip. It's here, for future me or others that come to this issue πŸ˜…

https://git.drupalcode.org/project/drupal/blob/11.x/core/modules/navigat...

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Yep, it is still needed.

To address my last comment, even when ✨ Create a generic system to provide menu based navigation blocks Fixed got in, we didn't change the way that we alter the Administration menu for use in the Navigation bar. We're still using hook_menu_links_discovered_alter, which alters the Administration menu (/admin/structure/menu/manage/admin) in all contexts. For instance, from /admin/structure/menu/manage/admin with and then without navigation module:

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I also noticed that I can drag the blocks in place on the left in the Navigation mode. Either right away, or after clicking 'Move' from the contextual links menu, I can only move things using the off canvas dialog that slides in from the right.

Firefox works as expected. Other layout builder pages also work as expected in Chrome, so it appears to be an issue on Chrome only.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Closed / re-opened the MR to trigger Tugboat. Sorry for the noise.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Merged to 1.x πŸŽ‰. Thanks!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Thanks for the merge conflict resolution @bronzehedwick. Looks good to me.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

This is great! Thanks. Merged to 1.x.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Postponing pending investiagtion on [#].441118

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Thanks! Merged to 1.x πŸŽ‰

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

m4olivei β†’ made their first commit to this issue’s fork.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

We're green now on the core MR!

There is one warning as previously mentioned, but no errors. That warning is out of our control for the moment.

I'm going to call this one done.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

The core inclusion MR is now green! Thanks for chasing all of these down @plopesc!

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Looks good. Tests passing and Tugboat looks fine. Merging and will update core MR.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Looks good. I've merged it. Will update core inclusion MR and then if we get a green result, we'll mark this issue as fixed.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Nearly there on the latest run!

https://git.drupalcode.org/issue/drupal-3438895/-/pipelines/145241

Just the "Validatable config" is showing a warning, but it's on to the test runs at time of writing 🀞. I'm out of race track for now though.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated patch and script. Adds some eslint globals.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated script.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Updated patch. The phpstan ignore path was incorrect.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I'm going to merge what we have here so far so that it can make its way to the core inclusion MR.

The things that are warning at the moment in our contrib CI is eslint, which is a known issue. Also phpstan and phpunit for the next major, which is expected per the conversation here, and I think we can deal with for now.

I'll leave this issue open in case there is more to address after the core inclusion MR is updated with the latest progress from here and from πŸ“Œ [No commit] Prepare for core inclusion Active .

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

Here is an updated script that.

  • Meant to run on an up to date checkout of Drupal core 11.x
  • Will establish the core inclusion MR branch from ✨ Add the new Navigation to core as an Experimental module Fixed on your local
  • Will clone the navigation contrib project into core/modules overwriting anything there
  • Will apply the MR patch from this issue. These are changes to navigation module source code required for core inclusion.
  • Will cleanup the navigation clone, removing unnecessary files and git history
  • Will apply the patch from #15 of this issue. These are changes to core files required for core inclusion.
  • Will run prettier and build:css to ensure source files meet core standards, although they should already.

After a script run, the changes necessary are all staged for commit. They should be reviewed, committed, and pushed to the core inclusion MR.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

We'll need a handful of changes only when navigation is living in the core/modules directory to outside drupal core files. Here is a patch for that. Not really a good place to put this as an MR that I can think of, but open to suggestions.

πŸ‡¨πŸ‡¦Canada m4olivei Grimsby, ON

I added OPT_IN_TEST_NEXT_MAJOR: '1' to the Gitlab CI configuration.

With that, the test suite runs against Drupal 11.x core and the error detailed in #4 no longer shows up. However, another error appears:

There was 1 failure:
1) Drupal\Tests\navigation\Functional\ShortcutsNavigationBlockTest::testNavigationBlock
Failed asserting that two arrays are identical.

This is checking cache tgs on the page, and there is a new unexpected cache tag returning. However we do have a comment there noting that we were anticipating this.

I think the contrib module needs to remain usable on the current release of Drupal (both for developers working on it and test run). I think for these test fixes, it's a case of leaving in the contrib module what satisfies the test runs on the current major, and addressing issues in the next major as a change required in πŸ“Œ [No commit] Prepare for core inclusion Active , in the MR meant to stand in as a patch for the sync script to apply.

I'll wire that up, and I'll push an update to the core inclusion PR.

Production build 0.69.0 2024