Account created on 24 April 2008, about 16 years ago
  • PHP / Drupal Developer at LullabotΒ  …

Merge Requests


Recent comments

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your suggestions.

To make the UX more consistent with other pages like Permissions, moved the filter textfield into a fieldset container and changed the label to Filter"

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

We are experiencing a similar issue, and the reason why we have prices with more than 2 decimals is that Commerce price widget allows to enter them. Product manager entered a 3 decimals price by accident.

Adjustments are more likely to have more than 2 decimals, but they are not throwing this error. Reason is that adjustments are rounded by the following call:
$adjustments = $this->adjustmentTransformer->processAdjustments($adjustments);
That internally combines, sort and round the adjustments.

So, it seems logic that unit item prices are rounded as well, as proposed in the patch.

Would be great if you could consider this patch, or propose a different approach.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Code updated against 11.x and upgrade path test coverage added.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Back to RTBC once the suggestions in #28 were addressed and MR is ready to be merged.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

I don't see any of the mentioned issues as blockers.

Thank you for stepping up @m4olivei!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Added reference to ✨ Add the ability to filter by field type when creating a new field Needs review that might be a good approach to work in parallel.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Ceated MR with POC to confirm if the feature is considered and the approach valid.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Hi! I understand the role and I'm happy to help help here. :)

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

@johnv I can confirm the bug and the error you mentioned "The time is incomplete. A value must be selected for hour." is not being triggered.

Instead, default value of 12 00 pm is saved, which is not accurate.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up!

1.0.0 release is available requireing commerce 2.37 to ensure the mentioned bug is fixed.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Back to RTBC once πŸ› drupalInstallModule nightwatch function does not work with Experimental modules Active was merged and the module installation process simplified.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created basic MR to address this bug.

I was not able to find tests to cover this Nightwatch command, so I assume that MR is ok as it is. There are no regression and experimental modules support could be tested once the ✨ Evaluate adding Nightwatch tests Active is merged.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

These changes are not affecting the code and tests are still green, so I'm moving it back to RTBC directly.

Thank you

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for pushing this one!

Checked code and found out that new approach is not respecting menu link attributes. Added extra tests to confirm it and avoid overlooking this again.

I don't know if the new approach clears up all the doubts about the use of stateless functions and how to handle the use of menu links in SDC expressed in the original version.

That is a bigger concern that not only affects to navigation, but also to how to render menus in SDC. A follow up issue to address this in a global and consolidated way could be worth here.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Initial approach moving the logic to a trait in the core namespace, so it could be used by Link module and others , avoiding to add the dependency on Link module.

A good candidate to make use of this trait would be πŸ“Œ Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review once that MR gets in.

Final trait placement and/or Name is open for discussion, but this initial MR is ready for review.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

All the MR comments have been addressed, follow up issue for validation referenced, code updated to latest 11.x and conflicts solved.

This is ready for a final round of reviews.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Code updated after πŸ› Custom Navigation logo is disconnected from new Layout template Fixed was added to core. Minor conflicts had to be addressed.

This is ready for a final round of reviews.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

@larowlan After searching through the issue queue, I have seen that πŸ“Œ Fix Block config entity type config schema violations: weight, property Postponed already exists and is the key to unlock validation in blocks. There would be no need to create a new issue.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the progress here!

I'm missing these 2 small tasks from the strategy defined in #37

  • Remove ID from h4and aria-labelledby from ul
  • Open a follow-up to add back the ID and aria-labelled-by with JS to avoid caching issues. Also in this follow-up replace the ID on the li items, r evaluate if they are needed.

Other than that, I think this is good to go!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

This MR is trying to address logo issues in parallel with πŸ› Custom Navigation logo is disconnected from new Layout template Fixed .

Problems with custom logo visibility are being worked on that issue.

The aim of this issue is to resize images, if possible, to fit in the 40x40 expected space for logo.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Test coverage for icon resizing on upload added.

I think this is ready for a first round of reviews.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Added a first POC of the new approach.

It will require some adjustments and tests, but I think it is working as expected.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good points @SKAUGHT

Let me try to clarify the approach to try to address your concerns.

The automatic resize image approach does not require the image module to be enabled because the image manipulation API is in \Drupal\Core\Image class.
If the Image manipulation API fails to transform the image due to the unlikely scenario where GD is missing or any other internal fail, we can show an error indicating that image has to be of the specified dimensions, like FileImageDimensionsConstraintValidator does.
If SVG support is added in the future, we might need to create a specific path for that

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Been thinking about this issue and I think that using Image Styles here adds an extra layer of complexity that might not be necessary.

Having an image style we need to assume these risks:

  • Image style could be modified by site administrators, leading to unexpected results
  • Image Style could be deleted by site administrators, so we cannot assume that it is present and need to have a fallback in every place we load the custom logo image
  • Would force to add anew dependency on drupal:image for Navigation

My proposal here is to use a similar approach as we already have when uploading a picture to an image field that exceeds the maximum allowed size for the field.

When image is bigger than 40x40px, it would be automatically scaled during upload and a message like this would be shown to the end user:
The image was resized to fit within the navigation logo expected dimensions of 40x40 pixels. The new dimensions of the resized image are 40x40 pixels.

Following this path, code would be much simpler, reducing the number of possible points of failure and we would be safer assuming that original image can be used directly as navigation logo.

To give more flexibility, the icon size could be set as properties in the navigation.settings config entity, but not shown. So contrib modules or advanced site administrators could modify them.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Removed a couple of unnecessary dependencies from test class.

I think it is safe to mark this as RTBC now.

Thank you for your efforts here!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Hello @Prashant.c

Thank you for reporting the bug. However, I'm afraid this is a duplicate of this layout builder issue: πŸ› "Unsaved changes" message incorrectly appears on layout builder Needs work

Regarding the needs to add links to go back to the site, there's already a discussion open in πŸ› Enhance Navigation admin workflow with Managed Tabs. Needs review .

Marking as duplicated. Feel free to reopen if you think this is not appropriate.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for working on this.

Took a look locally and icon logic works well.

Added some minor comments in the MR.

It would require a review from someone with better knowledge of the HTML structure to confirm that everything is OK.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Added basic test to ensure that HTML attributes are being placed as expected.

Agree that this might not be enough to test the CSS behavior, but it checks the HTML structure and would avoid unwanted markup regressions.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for taking care of the initial CR.

Took a look into that and made some minor changes to add some extra information.

Back to RTBC now.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your comments in the MR.

Worked on them and I think this is ready for another round of reviews.

Adding reference to follow-up created: πŸ“Œ Add a generic trait for logic to convert references into Urls in LinkWidget Active

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Resolved the question opened by @larowlan.

I think it is safe to move it back to RTBC now.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Added logic to get rid of the unused permission administer navigation_block.

Thank you for the guidance here.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads-up. I was not completely sure whether this change might require an upgrade path or not.

Added post_update hook that assigns configure navigation layout permission to roles with configure any layout permission. Those were the roles with access to modify the navigation layout, so the change will be transparent for them.

I did not add any replacement for administer navigation_block permission, given that it was not being used anywhere after the LB refactor.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you!

Tested locally and works as expected with the current icons approach.

I see 2 missing points though:

  • We need tests for this
  • We need to confirm that this approach is not creating conflicts with πŸ› Special Menu items are rendered as empty links in navigation RTBC , which is also affecting to toolbar-button.html.twig. However, we cannot work on those possible conflicts until one of them is merged
πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Icon config is consistent with the current icon approach.

There's no clear direction for final icons yet.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Added comment suggesting a different approach to set the <h4> title using the original logic we had in place.
Let's decide the preferred approach and wrap this one!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR looks good and S has been updated as requested in #36.
Marking as RTBC.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Tests added for this new scenario.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid


Tried to reproduce the bug against latest 11.x and I was unable to get the error.

I wonder why the error screenshot mentions big_pipe because that module is not being enabled in standard profile.
Also the error is apparently trying to find a theme called big_pipe, when big_pipe is a module.

It seems there are other reasons behind this error.

Could you please try again and reopen if the problem persists with more precise steps to reproduce?

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Adding πŸ“Œ Agree on a lazy load strategy Active as reference to deal with caching at user level issues.

My concern or question about how to integrate a menu here is because most of the links I think that could be added to this menu are user specific, like /user/XX/foo and I am not sure how we could handle routes like that from a menu.

Mostly thinking about menu links created from the UI by site admins, where it is not possible to indicate URL parameters.

Core's user routes provides shortcuts like /user or /user/edit that redirects to the user specific page, but that's something not all the contrib modules that provide user specific routes or views that use the user ID as parameter would support.

We could encourage the usage of modules like β†’ or β†’ for this menu, though.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for the heads up!

Planned to deal with this as part of the mentioned issue πŸ“Œ Investigate using the core "User account menu" in favor of custom Navigation Block for same Active once the related issues πŸ“Œ Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review & πŸ“Œ Rename UserNavigationBlock to NavigationUserBlock for class name consistency RTBC are in and the ground is ready to mess with it.

We can leave this open as a reference and reminder in the meantime, though.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

If I understood correctly, the bug reported is related to the need of creating manually the shortcuts when installing the module in minimal profile.

In that case, I believe the module is working as expected.

Navigation should not show the shortcuts section unless there are actual items for the user. It would be a useless item in the bar with no purpose.

Shortcut links are independent of navigation module, and this module can't take that responsibility, I think.
Minimal profile does not provide default links because it aims to reduce all the boilerplate content. Standard or Umami are more opinionated and generate more content by default.

Feel free to reopen the issue if I misunderstood the summary.

In situations like this, it's helpful to provide the expected behavior or a resolution proposal to have a better understanding of the scenario you find problematic.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your review!

I think the scenario described in your manual test is missing the step to create shortcut links.

When there are no shortcut links to show to the user, the shortcuts navigation block is not rendered.

According to Standard profile creates some shortcut links by default during installation.

It is possible that this is not happening in minimal, where you need to create some shortcut links by yourself to have access to the shortcuts block.

Could you please repeat your tests and confirm this?

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

During Drupalcon sprints worked on an approach where ID attributes, and all the references to them are removed from the HTML markup before generating the hash, so the problem is gone, but at the cost of some extra calculations that would be great to avoid.

Would be great to have some extra feedback about whether we can get rid of id attributes or should we explore other paths assuming that IDs could be there.

On the other hand, if we remove IDs from the core templates, but a specific site overrides the template making use of them, the navigation would be broken.

That's something that already would happen with current toolbar, but need to confirm if that scenario should be supported by navigation.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

We can close this given that πŸ› Regression: Shortcuts menu flickers when the page is loaded Fixed showed the way to avoid flickering with no need to make an extra effort at render array level.

Same approach could be followed in the future if necessary.

Talked to @mherchel in the weekly Drupal Admin UI call and he also agreed.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

MR created.

There is an, apparently, unrelated failing test. Will relaunch the test suite later.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you both for taking care of it!

I think we can mark this as RTBC.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Created MR that creates a MenuLinkTree manipulator that triggers an event that gives opportunity to other modules to react to the navigation menu build process.

Created an event subscriber that removes the Help and Content links from admin menu.

Doing it in this way, the original menu is not being altered, and provides an extensible system that allows other modules to interact with the navigation menu link tree.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

I really like the approach proposed in #7 or a similar one.

However, I don't see where we can connect the Icon plugins with the menu items.

Menu item class for menu items created through yml files are like toolbar-button--icon--system-admin-structure that can be inferred from the menu definition. It would require some Drupal knowledge to discover the specific class. These menu items cannot be modified from the UI though

However, if editors want to add menu items through the UI, icon class is like toolbar-button--icon--menu-link-contenta614ff3e-4306-4830-933c-f152268ea849 which is not friendly and also can differ between the different environments.
At this point 2 possible approaches come to my mind:

  • Add an extra config property in NavigationMenuBlock where editors can define the menu item - icon relationship manually. Values for core menu items would be shipped by default. For other NavigationBlocks like Shortcut or User, the icon could be chosen from the list of available plugins and stored as part of the block config.
  • Modify the menu item form UI to add a field from where each menu item could relate to an icon to be used just in case the menu item is used for navigation. These relationships could be stored as third party settings in the menu config entity. hook_install() would take care of setting the default values when installing navigation. For other NavigationBlocks like Shortcut or User, the icon could be chosen from the list of available plugins and stored as part of the block config.

Would be great to have some other ideas brought to the table and reach some kind of agreement before starting to work on this.

Thank you

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

As discussed with @ckrina in Portland, this new block will be hidden by now, but used for the Help Navigation item for testing purposes and simplify the footer logic.

Added functional tests to ensure link access rules are preserved.

Attaching screenshot to ensure that there are no visual regressions.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for working on this one!

Would be great if you could merge 11.x branch into the MR branch to fix the broken tests. They're unrelated to the issue, but to the specific 11.x HEAD you used as base.

While we are here, would be great if you could remove the moduleHandler property from the NavigationUserBlock class.

You can completely remove __construct() & create() methods. We could also get rid of the reference to ContainerFactoryPluginInterface in the class definition.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Postponing due to necessary implementation discussion.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

You were right.

Blocks need to be accessible for administrative purposes. Block itself has to ensure the logic in build() is safe enough and not rely on blockAccess() for logic that could throw unexpected exceptions.

Thank you for your help here.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your work on this.

The root source of this issue is that in LB managing page, apparently NavigatioShortcutsBlock::blockAccess() method is not being invoked.

That module checks whether shortcuts module is enabled or not before continuing.

Form this point I think we have some alternatives:

  1. Try to find why blockAccess() is not being invoked and try to invoke it even for the LB config page
  2. Add an extra check in NavigatioShortcutsBlock::build() to ensure that Shortcut module is enabled
  3. Continue adding shortcut as a navigation dependency. If so, I would remove all the code checking if Shortcut module is enabled

From my perspective, I would avoid the 3rd option because that would add an unnecessary dependency for the future.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Code looks good and the administrative UX has been improved.
Also provides tests.

Thank you!

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your efforts here.

Unfortunately, navigation_top_bar is just an instrumental module to prevent access to the experimental top bar unless site administrators assume all the possible risks.

It means that all the code and logic should remain in the navigation module to make the transition form experimental to stable as simple as possible avoiding BC breaks.

Discussion about strategy and implementation can be found at πŸ“Œ Create feature flag module to control navigation top bar visibility Fixed .

Marking it as won't fix because moving code to navigation_top_bar module is discouraged.

Selector improvements in the test are nice, but I believe we can introduce them as part of a different top bar related issue, like πŸ› Navigation Top Bar hides entity local tasks even if the user has no access to the bar Active

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Closed MR against contrib navigation module. Created equivalent MR against core.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Moving to NR.

Instead of adding a pre_render callback, opted for adding a simple placeholder when user is not allowed to see the navigation bar because it's less intrusive with the LB render array generation process.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Good catch πŸ’―

Added logic to include the font if only the toolbar is being rendered.

About font split. Do you think it has to be addressed here? Or might be moved to a lower priority follow up?

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

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

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for being able to capture the glitch!

For reference: This flickering was introduced as part of this comment in the main core

Need to find a way to have a cacheable placeholder.

We could consider this a duplicated of πŸ“Œ Agree on a lazy load strategy Active or vice-versa.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Thank you for your reviews.

Navigation already provides a basic hook_requirements() implementation:

If you think it can be improved, or is not descriptive enough, we could update it.

πŸ‡ͺπŸ‡ΈSpain plopesc Valladolid

Hi @bnjmnm

The reason why it failed is because the --NUMBER suffixes added to the IDs modify the HTML markup, hence the hash generated from that markup is not consistent.

This is because Html::getUniqueId() methods adds a random identifier for AJAX calls

Here are the steps to reproduce it:

  • Replace clean_id with clean_unique_id in navigation-menu.html.twig
  • Clear Drupal cache
  • Clear browser local storage
  • Disable render cache from /admin/config/development/settings or config files
  • Reload the page and confirm that there's an AJAX error and navigation menu nested levels are not being loaded.

Getting rid of the id suffix solves the issue in the meantime, even if the HTML markup is not correct. It has to be resolved somehow, though.

Another option would be to get rid of id attributes in the template, but I'm not sure if that's possible for accessibility reasons.

Production build 0.69.0 2024