🇮🇪Ireland @lostcarpark

lostcarpark → created an issue.

I've done some optimisation of the SVG file, and got it down to 2.5KB.

lostcarpark → created an issue.

drumm → credited lostcarpark → .

I have uploaded PNG and SVG versions of the logo, both sized to 512x512 pixels.

Here's a preview of the PNG. I had to give the SVG a .TXT extension, so I don't think I can embed here. However they should both look the same unless you zoom in.

In terms of file size, the PNG is 9KB, while the SVG is 5KB. Both are under the Project Browser requirement of 10KB.

I would favour using the SVG since it's smaller, and we already use a SVG for the module default image.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue. See original summary → .

lostcarpark → created an issue.

lostcarpark → created an issue. See original summary → .

I'm relatively new, but if I can help, I would be happy to be added to the list (or also happy to help without being on the list). I've run a couple of the meetings in the past.

Updated .gitlab-ci.yml to delete recipes before running eslint. Checks are now passing.

Ready for review.

lostcarpark → made their first commit to this issue’s fork.

This is partly my fault. I fixed up the tests to work for the drop-downs. Rather than just plow in and work out selectors for the new markup, I should have asked can we make the checkboxes easier for the tests to select. I'll know better next time.

I have reviewed the changes and everything looks good to me. The revised selecters look much cleaner. I've also carried out a manual test, and everything seems to work correctly.

I note that the eslint check is failing due to the recipe.yml file. This file has not been touched by this change, so I think it's an unrelated issue, so I think it should be fixed in a separate issue.

I'm still in favour of being consistent with Drupal terminology. If the Drupal term is "Apply" we should use it. If we think that's going to be confusing in Project Browser, we should petition to have the term changed in the rest of the Drupal project.

Also, I think an important distinction with recipes is that they can be re-applied multiple times (I think the current Project Browser implementation blocks this), and they can't be "uninstalled". I think if we use the same install/uninstall terminology we use for modules, it masks this distinction.

I think Simplenews makes sense as an initial focus.

However, as Drupal CMS evolves, it would be great to offer options for other solutions, whether they are alternative Drupal solutions (I don't have a specific one in mind), locally hosted options, such as Mautic, or SAAS solutions like Mailchimp.

I would guess a separate recipe would be needed for each option.

Got to agree that improving the description for the cookie option on the settings form would be really helpful.

Rebased for latest changes. Tests passing. Manually tested, and verified appearing correctly for modules with a single install.

As far as I can tell, Package Manager is still in all branches of Automatic Updates 3.x. Adding the Automatic Updates project with Composer, and enabling the Automatic Updates module should enable installing in Project Browser.

I presume there will be a version of Automatic Updates that removes Package Manager, but that doesn't seem to exist yet.

The initial check-in was just some reorganisation of the ToolsMenuDeriver class so that child menus are added by logically named methods rather than a single monolithic one. I created the MR just to check that the change still pass the automated testing before starting on new functionality.

The latest commit adds child options to the "People" menu, including listing all the roles under the Rolls option, as well as adding options to the "Config->People" menu. The "Manage user fields", "Manage user form display", and "Manage user display" options ought to be 4th level items, but I've placed them as 3rd level for now.

Yes, the require-dev ensures the Devel module gets loaded on development systems to allow the automated tests to run. There is a test that checks the Tools->Development menu gets created if the Devel module is enabled.

If you add this module to your site with composer require, it should not add the Devel module, so it should not get added to production sites.

Enabling level 3 menus (grandchild items) would be extremely useful to the Navigation Extra Tools module.

I'm working on replicating functionality from the Admin Toolbar Extra Tools ✨ Add bundle derivatives like admin_toolbar_tools Active module to place entity items as sub-items in the Navigation menus.

For example, the level 1 Content Types menu gets expanded to show a list of content types as level 2 items, which would expand to options like "manage fields" and "manage display" as level 3 items, if those were available.

I think the level 2 items are still helpful, but we're dependent on this issue to provide the level 3 items.

Currently, Navigation is limited to a maximum of 3 levels. This is going to limit us, as Admin Toolbar Extra Tools adds many 4th level menu items.

Where practical, I can add them as 3rd level items for now. For example, Admin Toolbar Tools adds items for managing fields and display under "Config->People->Account Settings". For now, I've added these items at the 3rd level for now.

However, there are other 4th level items where this isn't practical. For example, a third level item for each user role is added under "People->Roles". Admin Toolbar Extras adds "Permissions", "Delete", and "Devel" options as 4th level items under these. I think trying to add these at the 3rd level would be unwieldy, so I'm deferring for now.

There is an issue open to add 4th level menus to Navigation 💬 Support Deeper Navigation Levels Active , but it doesn't look like much has been done on it yet.

For now, I think we'll just have to live with this limitation.

I'm working on this issue now.

Accidentally merged to 1.0.x branch instead of 1.1.x branch. Reverted and merged to 1.1.x.

I've checked the CSS styles, and as far as I can tall all the pb-module-page styles are still required for the Modal Detail page. However, I have renamed them to pb-detail-modal, which seems more appropriate.

I think we are ready for another review again...

Rebased to include latest changes. As the number of commits in this issue has grown, the rebase is taking quite a long time, so hopefully we can avoid too many more!

I've removed ModulePage, and stripped everything relating to it out of App.svelte. It makes it quite svelte!

A possible future enhancement would be to make the pop-up update the address bar and give the detail modal a unique URL.

I agree with Chris, the Queue/Dequeue issue is no also present on the existing 2.0.x branch, so not caused by this change, so I think it should be dealt with by a follow up issue.

Thanks for working on this. It looks great. I just wonder should we be making the permissions more granular, so that users could be given access to some functions but not all?

I think that's a great idea, but definitely a separate feature request. If you would like to get the ball rolling, would you like to open an issue?

I discussed on the #admin-ui channel on Drupal Slack in this thread.

At present the Navigation do not want links added to the footer menu, and consider it a "constrained space", and I don't want to disrespect that.

I'd appreciate if anyone with interest could read the thread above and add any thoughts to this issue.

I've merged this change to the 1.1.x branch. There are a couple of other changes I'm aiming to complete before a 1.1.0 release.

Hopefully ready for review again.

Thanks for the feedback.

I've also noticed that issue with the way devel dumps variables, and I'd agree it's a devel issue rather than anything this module can consider.

Regarding the devel toolbar, I notice it's discussed in this issue. The maintainer seems reluctant to do anything in the devel module. Can definitely consider it in a follow-on issue.

Add instructions for showing running tests in browser.

@debrup, the feature request was to replicate the development menu that the Admin Toolbar Extra Tools module adds. I think we should limit the scope of this issue to that functionality.

Replicating the functionality of the Devel Toolbar, sounds worth investigating, and adding options like routes_info certainly sounds useful. I'd like to check that the devel team aren't already working on that functionality.

I would favour merging the current change, and opening a follow on issue to look at the possibility of incorporating features of Devel Toolbar.

Feel free to open that issue if you feel inclined.

I've added a test to simulate opening a modal detail page for a module, closing it, opening then closing the detail for a different module, and finally reopening the first module's modal.

All tests are passing.

I realised that with a very small change to the fixture, we could get the module with 1 install onto the front page, simplifying the new test considerably.

Unfortunately, I ended up having to make a lot of small fixes to other tests. I think it's worth it, though.

I considered adding a "0 installs" test case, but I realised that when a module has no installs, there is no install count label at all. It might be worth adding a new test for that, but it seems out of scope for this issue. I will look at opening a follow on issue.

All tests are now passing, so moving to Needs Review.

I had one more thought this morning, would it be worth creating a new test to open the modal, close it, and open it again, so that the use case of reopening the modal details is in an automated test?

I had a look at the "View commands" popup, and it comes from a different component. I feel that either changing the width or adding an aria tag would be outside the scope of this issue.

Definitely agree that the search options currently take far too much space.

Added "max-width" of 80rem to project-browser-popup class.

Added aria-haspopup="dialog" to the project button.

All tests now passing.

I think this is ready for review.

Ah, it looks like these changes were already made in 📌 Disable DrupalCI Fixed , so I don't think there's anything left to do.

Updated fixture to make the total installs of the Grapefruit module 1. Required some minor adjustments to existing tests.

Added testInstallCountPluralFormatting test to search for Grapefruit and check install count text is "1 install". Also checks for Octopus module ("235 installs").

I used an xpath query to check that against the complete text on the element, as just checking for "1 install" in the output would also pass for "1 installs".

Looked at this this morning, and asked, "who put Kev-Value in the title?", and it appears that I did.

I believe I've identified the cause of the problem that was causing the modal dialogs for each module fail to open a second time.

In Project.svelte the <div> for each modal was being created when the component initialized.

In the handler for the module title, the <div> was being opened in a modal.

In the close handler, the element was being destroyed.

When the title was clicked again, the element no longer existed, so it couldn't be displayed in the modal.

The fix is to move the creation of the <div> element into the click handler for the module card title. This allows it to be recreated every time the title is clicked.

This also means the element isn't created until it's needed, or never if the titles aren#'t clicked, removing some processing from the page load.

Oops, messed up a screenshot in the above.

I did some minor tweaking of the legend, wrapping them in a <div> to group them together.

In desktop view, this groups them together cetnrally, which I think looks better:

In tablet view, the module total is on one line, and the legend on the next:

Finally in mobile view, the the number of modules, and both legend items are each on separate lines:

I think this is ready for review now.

Hi @debrup, when assigning and issue to yourself, it's considered good queue etiquette to post a comment stating what you intend to do with it, and how long you expect the assignment to last. Usually this would be to work on the issue fork. It wouldn't usually be necessary to assign the issue to carry out a review.

I've tweaked the layout to put the legend between the number of results and the grid/list buttons:

On mobile devices, it puts each item on its own line:

However, on small tablets, I think it needs some work:

I'll work on it some more, but feedback on work so far would be appreciated.

Thanks for tidying up, @ressa!

I'm aiming to work on this soon, but as a first step, I've added functionality to add the Development menu from the Devel module to the Tools menu, under issue ✨ Expose Devel menu to inside the Tools menu Active .

As a lot of the code is common with this change, I'm waiting for it to be merged to start on this one.

One issue with the Navigation menu currently is menus with submenus are no longer clickable, as top items with submenus only open and close the menu on the level below. This would be a problem if adding a list of views under the "Structure->Views" menu item would make the Views menu no longer selectable.

My current thought is that Navigation Extra Tools would add a new item, "Views administration" under the current Views item, that would take you to the same route, then add the list of views below that. That would mean one extra click to get to the Views admin page. Would that be an bother people?

I might also add "New view" on that list so you could skip the views admin page and go straight to add a new view.

I've renamed the "ExtraTools" class to "ToolsMenuDeriver", which I think better describes its function. I've also added weights to the added menu items.

I've added a basic test that checks that the Development menu isn't present when Devel module isn't installed, and gets added after installing Devel.

A lot of the sub-menus depend on other modules, so I may add some further tests to test menus are only available when the expected modules are enabled.

I think this is ready for review. It would be helpful to have feedback from people using the functionality in Admin Toolbar.

Just a note that because of the way Navigation renders menus with children, the Development menu itself isn't currently reachable. It just expands to reveal the submenu items. Since this points to /admin/config/development, which just shows the development menu page, I don't think it's a problem.

I have added the functionality to add the Development menu to the Tools menu.

It seems to be working, but I'd like to add some tests.

Feel free to try out in 1.1-dev in the meantime. Feedback very welcome.

Thanks for all the information.

This is something I want to work on, and adding just the Devel menu seems and easier place to start than all the entity menus the Admin Toolbar Extras module implements.

Added section about ddev-drupal-contrib.

At present the "maintained" icon is not in the repository, but there is an issue open to restore it, 📌 Replace Wrench icon on project views with Gear icon Postponed .

This issue is dependent on that one.

I've reviewed the code, and I've queried one thing. I think at present, a module will only showed the maintained icon if it is also covered by the security policy.

I've tried testing manually, but at present I'm only seeing data if I turn off the "maintained" filter, so I think there may be an issue with the back end at present, but because of this I wasn't seeing any "maintained" icons. I think this is a data issue rather than an application issue.

lostcarpark → changed the visibility of the branch 3282163-improve-iconography-usability to hidden.

The path to the icons was using a non-existant variable for the site URL, so I removed that as I don't think it's needed, since the icons are on the same server as the page.

That brings back the blue security icon, but not the green wrench, as it's not currently in the images folder.

It's presumably not showing on the module cards, so a decision is needed on whether it should be there. If it's not on the cards, there's no need for it on the key.

Still to do: tidy up the formatting, move the CSS out of the Svelte styling.

I don't know if this is correct, but the extra list/grid buttons don't seem relevant to the topic of this change, so I removed them. If we want to make a change to those buttons, we should open a specific issue to deal with that. Tests are passing.

This change seemed to be quite a long way behind, so I rebased to the current 2.0.x branch.

All tests are passing, except the "next minor" Nightwatch test. I think this is because of the change to Nightwatch 3.7.

I carried out some manual testing, and found a few minor issues.

The icons for the keys are not currently appearing. The URL in the <img> tag is undefined/modules/contrib/project_browser/images/blue-security-shield-icon.svg, so something seems to be going wrong with generating the location.

Also, the key elements seem to take a lot of space. I think they could be spread out over a single line, perhaps in a flexbox.

There also are two sets of list/grid buttons. Not sure if that's something I accidentally introduced in the rebase. I'll have to check the current 2.0 branch and make sure it's not in that.

The other thing is this change uses a <style> section in ProjectBrowser.svelte. As far as I know we're not using Svelte styling, and have moved styles into pb.css to be more consistent with Drupal. Is this right?

lostcarpark → made their first commit to this issue’s fork.

Changing version to 1.1 as new feature.

As this is a new feature, I'm flagging it for consideration for version 1.1.

Issue successfully merged, and all pipelines passed.

This looks good to me. @prem suthar would you like to review and set to RTBC if you are happy?

Successfully merged. Thanks all for contributing.

Thank you both for working on this.

@prem suthar It's generally considered proper issue queue etiquette to not work on an issue someone has assigned to themself. If you were working on it, before they started, assigning to yourself and leaving a comment when you start is the best way to let other people know you are looking at it.

Could you review @sayan_k_dutta's MR !14 and move the issue to RTBC, then I can give you both contribution credit.

Thank you both for looking at this.

@prem suthar, yes there were two menu items with the same weight, but they each have different parents, so I don't believe it's a problem.

I don't think any items with navigation_extra_tools.help as their parent should change.

I think the only change should be to items with a parent of navigation_extra_tools.flush.help. The weights of different levels of menu are independent of each other, but if you want to ensure every item has a unique weight, you could start the weights of the submenu items at -19 instead of -9.

Setting back to needs work for now.

Thanks for working on this. Change has been merged and will be included in the next release.

Update...

It seems to be behaving itself now.

I don't think anything on the site has changed apart from adding more content, and probably clearing cache a few times (but I had cleared cache when it wasn't behaving, and it didn't seem to make any difference.

I suggest keeping the issue open a few days to see if it occurs again, but if it doesn't, I'm happy to close it.

lostcarpark → created an issue.

Assigning credit.

Thanks to @ultimike for code reviewing and opting module into security policy.

Stable release 1.0.0 has now been added.

Tests passed. Merged change. Marking issue fixed.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

This is a great point. I agree, the tools would see to fit better with the items in the footer than the rest of the navigation bar.

I think it would be nice if it was something the site builder can decide on, so adding a configuration to allow this would be useful.

I'm going to focus on getting a 1.0.0 stable release, then I will look at this.

Thank you for raising this issue.

I didn't realise how much admin_toolbar_tools does when I started this, and I was a bit surprised to see the plugins as part of it. The main functionality I was missing was clearing caches and running cron, so that's what I focused on.

I was intending to go back and figure out what the plugins were doing. I had made lots of use of the submenus under Structure, but never realised they were coming from admin_toolbar_tools!

As this adds a bunch of submenus, I'll need to make sure it does it in a way that keeps the higher level menu items selectable, as I've found Navigation tends to make items with submenus only open and close the submenu, and not be selectable themselves.

I think my priority is to get a 1.0.0 stable release, but this definitely looks like a feature worth adding for 1.1.

I like the improvements in cna.module, but I think there are still a couple of issues in both the cna_comment_insert and cna_comment_update functions.

You currently check if the type is a node, which is good, and set the $node_title and $to, if it is a node. However, there is no else condition, so it will continue to attempt the email sending with those variables unset.

For the node title, you could use the label() function instead of the title property. I believe this is recommended practice according to this change record → (though it's rather old, so I'm not 100% sure it hasn't changed). label() is generic to all entities, while title is specific to nodes, so you no longer need that in the if statement.

Rather than checking if the entity type is a node, I recommend checking if its an EntityOwnerInterface. That will mean the module will work for any entities with an owner.

If it isn't, it means the entity doesn't have an owner, so there's no one to send an email to, so you should skip the email sending altogether.

You could use something like:

if (!($node instanceof EntityOwnerInterface)) {
  // Entity has no owner, so no need to send email.
  return;
}
$to = $node->getOwner()->getEmail();