Grant credit to issue contributors.
My apologies, I missed it. In the past, those checkboxes were marked by default when contributor created a MR.
Reopening the issue to set the correct attribution.
Fixed
Merged
MR created
Great job here!
Thank you
Marking as RTBC, we need now the framework manager review for the new Twig function.
Thank you!
Created 🐛 Mismatched entity and/or field definitions: The User ID field needs to be updated. v2 Needs review to deal with the error mentioned in the last comment.
Created MR
plopesc → created an issue.
MR created using DI for the rounder service.
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"
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!
Code updated against 11.x and upgrade path test coverage added.
plopesc → made their first commit to this issue’s fork.
Back to RTBC once the suggestions in #28 were addressed and MR is ready to be merged.
I don't see any of the mentioned issues as blockers.
Thank you for stepping up @m4olivei!
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.
Ceated MR with POC to confirm if the feature is considered and the approach valid.
Hi! I understand the role and I'm happy to help help here. :)
Last patch also fixes 🐛 '00' minutes are considered as empty in OfficeHoursDateList::validateDateList Closed: duplicate
Adding for reference.
Duplicated of [##3455363]
@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.
Thank you for the heads up!
1.0.0 release is available requireing commerce 2.37 to ensure the mentioned bug is fixed.
Back to RTBC once 🐛 drupalInstallModule nightwatch function does not work with Experimental modules Active was merged and the module installation process simplified.
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.
- Created follow-up issue 🐛 drupalInstallModule nightwatch function does not work with Experimental modules Active to deal with issues enabling experimental modules in Nightwatch tests
- Moved some reused selectors to constants
- Added references to followup issues in @todo comments
These changes are not affecting the code and tests are still green, so I'm moving it back to RTBC directly.
Thank you
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.
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.
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.
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.
@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.
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!
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.
Test coverage for icon resizing on upload added.
I think this is ready for a first round of reviews.
Added a first POC of the new approach.
It will require some adjustments and tests, but I think it is working as expected.
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
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:imagefor 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.
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!
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.
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.
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.
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.
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
Resolved the question opened by @larowlan.
I think it is safe to move it back to RTBC now.
Thank you!
Added logic to get rid of the unused permission administer navigation_block.
Thank you for the guidance here.
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.
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
Icon config is consistent with the current icon approach.
There's no clear direction for final icons yet.
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!
MR looks good and S has been updated as requested in #36.
Marking as RTBC.
Tests added for this new scenario.
MR created
MR merged. Thank you!
Hello @SKAUGHT
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!
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 https://www.drupal.org/project/user_current_paths → or https://www.drupal.org/project/me_redirect → for this menu, though.
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.
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!
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 https://git.drupalcode.org/project/drupal/-/blob/11.x/core/profiles/stan.... 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!
Tests added to MR !7944
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.
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.
MR created.
There is an, apparently, unrelated failing test. Will relaunch the test suite later.
Thank you both for taking care of it!
I think we can mark this as RTBC.
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.
plopesc → made their first commit to this issue’s fork.
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
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.
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!
penyaskito → credited plopesc → .
Working on it!
Postponing due to necessary implementation discussion.
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.
Created 🐛 BlockComponentRenderArray::onBuildRender() does not check block access in Preview mode Active
To try to address the LB preview issue.
If that gets in, this one could be closed.