🇮🇪Ireland @lostcarpark

Agree SEO makes sense.

It does seem to have an extensive API, but the same is true of many modules. The description of the Developer Tools category is "Empower developers with tools that assist with developing and debugging the frontend or backend of the site". I believe that this refers to modules that explicitly provide tools to aid development, such as debugging tools, not just modules providing APIs or libraries.

I would be happy with SEO being the only category for this module.

My understanding of this module is that it replaces the standard Exposed Filters on Views with an enhanced version with improved functionality.

I don't see how it fits in Content Editing Experience. Could you explain this please, @bhogue?

I can see that exposed filters are a form of content search, but I'm skeptical that this is what someone would be looking looking for when browsing under the Site Search category.

The one category I'm happy with is Content Display. The module improves filtering options when content is displayed in a view, so I suggest that should be the primary category. I've updated the summary to move this first.

Leaving at Needs Review for now.

Agree with bsnodgrass.

SEO is clearly the main function of the module.

Administrative tools is also a good fit.

I was wavering on Automation Tools. Ultimately, it is automating the process of generating URLs for pages. However, I'm skeptical that someone is going to be thinking of automating URL generation when they look in the automation category, so I can't see it being useful to add it there.

Agree Administrative Tools makes sense, and I really don't think a secondary category is of much benefit.

Moving to RTBC.

While the old category, Path Management, does describe the module well, I think that category name was not very helpful to novice users.

I agree with @bsnodgrass that Site Search and Site Structure aren't a good fit. It may help with getting the site visitor to the correct page, but it has very little to do with the actual search process. Similarly, it may help with creating URLs that reflect the site structure, but the module itself has nothing to do with structuring content.

I would suggest making SEO the primary category, since the module helps ensure a single canonical URL for a page, which is an essential part of search engine optimization.

I agree Administrative Tools is also a good fit, but I would suggest making it the secondary category.

Updating the issue summary to swap the order. Keeping at needs review.

I have made a number of changes:

• Remove hard coded background colors where possible, especially from list view cards.
• For filter bar, replace hard coded light background with one that is 50% gray with a high transparency, so on a light background it will be slightly darker than the surroundings, and on a dark background, it will appear slightly lighter.
• Set buttons to use the core button style, so they will be consistent with theme buttons.
• Set links to use the core link class, so will use the theme link styling.

The changes are designed to minimize barriers to adding project browser support to themes. However, any theme specific changes should go in the theme rather than Project Browser.

Also, I see your point about removing the config flag. I suggest opening a separate issue to look at that.

Okay, I see this now. I had seen that the "Automatic Updates Extensions" adds a "Update Extensions" tab, but I missed Automatic Updates changed the handling of the route.

I will look at adding a check for the Automatic Updates modules are installed.

A lot of issues were fixed in the CSS refactoring that was done a while back, but there are a number of minor issues that I think can be easily fixed.

Starting a new branch to look at them.

MR !508 ready for review.

I have extended the previously added functionality to disable the "Update" page, which allows modules to be updated using tar files.

I have also extended the tests to cover.

lostcarpark → changed the visibility of the branch 3437724-unset-variable to hidden.

lostcarpark → changed the visibility of the branch 3437724-disable-update-option to hidden.

Added child issue for UK host Mythic Beasts.

lostcarpark → created an issue.

lostcarpark → created an issue.

lostcarpark → created an issue.

Brilliant! I will have to try out the 2.x branch soon.

Thanks for this feature.

Good work, @Shubham_kumar and @mahtab_alam. You seem to have both submitted very similar changes.

However, the "Update" should only be disabled if the "Disable add new module" config setting is selected.

I note that you've used a different hook to the way "Add new module". I may have just not found that hook. I don't mind which hook we use, but I feel we should be consistent.

I also think we should disable the route, not just the menu option, so that people can't get sent to the page another way. I implemented an event subscriber to do that for the Add New Module page.

One problem I had was getting the disabled menu option to stop showing up in Admin Toolbar, so please make sure your change works there.

Finally, I added the "DisableAddNewModuleTest" functional tests. It would be nice to see the "Update" option tested there.

Have a look at the previous change ✨ Add "Disable Add new module page" option to settings page Fixed .

Thanks for working on this!

We want the "Available updates" page (/admin/reports/updates).

However, the "Extend->Update" page (/admin/modules/update) allows module updates to be installed in a non-Composer way, so I think it should be hidden.

Need to check how the Automatic Updates module handles module updates, and whether it replaces or modifies this page.

Great to see this one merged... I'd almost forgotten I'd worked on it!

There is an open issue, 📌 Remove Tabledrag's jQuery dependency Needs work , regarding the Drupal TableDrag dependency on jQuery.

It probably won't be possible to completely remove jQuery until that's resolved, but it should be possible to refactor to use standard JavaScript for everything except the TableDrag calls.

lostcarpark → created an issue.

Resolved PHPCS errors.

After fix ESLINT is passing.

Unfortunately there's another PHPCS issue, and I think this one is on us. Opened 📌 PHPCS errors from 3310884 Active to look at that.

lostcarpark → created an issue.

lostcarpark → created an issue.

I really like the multi-select drop-down too. It strikes me as a much cleaner interface. I also like the way in the example above example, the number of entries in each category is shown. This would be especially useful if it showed totals within the context of the current filters, so if you have "covered by security policy" enabled, it will show the total number of modules covered by the security policy in each category.

I don't think the constructor is needed, because there's already one in one of the inherited classes. I have added a create function.

I have changed all \Drupal calls with dependency injections, and PHPStan is now passing.

Moving to Needs Review.

lostcarpark → created an issue.

I have rebased, but I noticed the compiled Svelte before wasn't minimized. After my recompile, it's minimized. Checking if there's an update to the compile procedure I've missed.

Needs a rebase and Svelte rebuild to fix the merge conflict.

There are some ESLint and PHPStan errors, but they don't appear to be related to this change.

I will try to rebase today, then happy to mark RTBC.

Confirmed "composer (next major)" is loading the required modules with the lenient endpoint.

Marking RTBC.

Uninstalled 1.0 release. Installed dev release.

Markdown text format now created:

I think a new release is needed!

Confirmed that "Markdown" text format not created for me.

After installing module on a new Drupal 10.2 site, I only see the following formats:

I can add a new text format and enable Markdown Easy filter, but I don't get the text format by default.

I have generated two PDFs.

The first is the banner as it appears in the image above.

The second has extra bleed around the edges, so that's probably the one we want to print from. It has white crop marks at the corners indicating the where it should be cropped.

Setting to Needs Review as we need to get this over the line if it's going to be at DrupalCon Portland.

Fixed long line and whitespace issues.

Most of the changes are fairly self explanatory, except possibly the removal of a line of commented out code from src/Plugin/views/argument/Date.php.

I checked git blame, and it reported the line was commented in 2022, so at present it seems unnecessary.

lostcarpark → created an issue.

Great to see CSpell issues getting fixed.

A couple of things that might be worth looking at in follow on issues...

• We should make cspell mandatory so that new spelling errors can't be ignored. We can do that as follows.
  

  cspell:
    allow_failure: false

• The global ignore list is handy for words that we want to use in the project that aren't in CSpell or Drupal's dictionaries, but we should be cautious of letting it grow too much as it could get out of hand if the project expands. Where a word is only used in one file, it might be more appropriate to move it to a // cspell:ignore line at the top of the file where it's needed.
• Adding variable names to the ignore list is a handy quick fix, but where possible we should look to make variable names more meaningful.
  • For example there are number of variables that are a combination of two words, such as $byhour, which could be separating the words in either snake case or camel case: $by_hour or byHour, making them easier for humans to read, and allowing CSpell to treat as two words.
  • Other variables, such as rdata, could be made more descriptive - presumably the "r" stands for something.
• If we want to exclude whole files, we could add to a _CSPELL_IGNORE_PATHS variable. Some modules exclude the README file, though disabling for just the maintainer section seems a good solution. It can also be useful for text fixtures.

lostcarpark → created an issue.

Fixing "var" statements, and changing "let" to "const" where not modified, reduces the number of errors considerably.

✖ 247 problems (204 errors, 43 warnings)

lostcarpark → created an issue.

So here's the turquoise version with the latest changes...

I agree the QR code overlapping the Drupalicon was a bad idea.

I've moved the text boxes down a little, moved the QR down a bit, and made the QR and Drupalicon both a bit smaller, so it's no longer overlapping.

I've done it on the green version because that's what's currently set up, but I can produce for a different background colour if that's the consensus.

I must admit I'm warming to the turquoise version.

Explained phpstan path change. Seeing back to needs review.

Changes made for GitLab to run without DrupalCI.
FunctionalJavascript test failed, but seems to be unrelated to this change (reporting about click that would land outside intended element).

After removing the cd line from the before script, the CSpell task completed successfully.

lostcarpark → created an issue.

Fixed missing spaces around operators.
There were several instances of 24*60*60 so replaced with a "ONE_DAY" constant. Also added "TWENTY_THREE_HOURS" and "FIFTY_NINE_MINUTES" constants for improved clarity.
There were also a couple of cases of 86400 that I replaced with the ONE_DAY constant.

After these changes, PHPCS reported the following:

A TOTAL OF 145 SNIFF VIOLATIONS WERE FOUND IN 42 SOURCES

I carried out some testing, and didn't notice any issues, but I certainly don't know all the ins and outs of the module's JS code.

It shouldn't make a difference unless the code is comparing variables of different types, which should be avoided.

After this change, errors further reduced as follows:

346 problems (303 errors, 43 warnings)

lostcarpark → created an issue.

Actually I misrepresented the number of errors. Eslint runs twice with different parameters, and the first run returns the bigger number of errors.

Before this change, the number of errors was:

555 problems (512 errors, 43 warnings)

After, its:

385 problems (342 errors, 43 warnings)

So a solid step in the right direction, but quite a lot to do!

Setting to needs review.

Additional issues will be created for further eslint problems.

This issue reducces the total number of eslint problems from 261 to 134:

134 problems (95 errors, 39 warnings)

This only deals with camel case issues in the JS files, which should make it easy to review.

It does include some minor indentation problems in YML files, which are also reported by eslint. I felt these were minor enough, and in separate files from the camel case problems, so could be included in the same issue.

I've also set the stylelint check to allow_failure: false, which will prevent future CSS changes that don't pass from being merged.

Setting to "needs review".

I have applied stylelint fixes by running ddev stylelint --fix.

This has mostly just corrected style ordering, but there were a couple of cases of "0" being used with units, and a first-letter pseudo-class referenced without double colon.

I don't think these changes should change how anything is displayed, but it would be prudent for someone more familiar with how it should format than me to give it a quick visual inspection.

Thanks for all the feedback!

@ashrafabed: 1. I do take your point on the horizontal title being easier to read/process. Happy to go that way if others prefer it, but I feel it's trying to balance readability and impact. Mentor/mentoring appears many times on the banner, so hopefully people will pick it up.
2. Yes the logo take up space, but I think a lot of the space at the top of the banner is above people's eyeline, so we don't want too much up there.
3. QR code is a great idea. I've added one to the URL suggested. If we adopt that, we'll need to get a redirect or a landing page set up.
4. Thank you.

@brianperry: I agree the green is dominant. I think the green DrupalIcon logo is a long established mentoring tradition, so I was basing it on that.
For the next version, I have produced three colour schemes, one in traditional Drupal blue, one in a turquoise that sits somewhere between Drupal blue and Mentoring green, and the third in full on mentoring green. I've kept the green Drupalicon. I feel that the blue one is doesn't go well with the Drupalicon. The turquoise one works better, though I still like the green one.

@kwiseman: I like the idea of getting them produced as bookmarks as well. It would be nice to have something to give people visiting the booth.

@mike.roman: Agree skill set sounds better than skill-set.
Good point on the slack logo. I think specifying that it's "Drupal Slack" makes sense. The old banner had a Twitter logo. I changed, partly because I didn't want to get into a debate on the old vs new Twitter logo, and partly because I don't think we're using it very much, and Slack is where the main action happens.

I've done some refining of the text. I'm not sure it's all an improvement, and I'd welcome suggestions.

With blue background:

Turquoise background:

Green background:

lostcarpark → created an issue.

Moving to RTBC, also creating some follow on issues.

lostcarpark → created an issue.

lostcarpark → created an issue.

I've added back the "previous major" test. I added sections for the composer and phpunit tests for previous major to explicitly set the PHP version. These should be removed after Drupal 11 becomes current.

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

There are some other ideas in the related issue in the Project Browser queue: 📌 Better Exposed Filters Project - Update Logo RTBC

If you would like any adjustments to one of the logos there, please post on that issue.

If you have ideas for a different design, please also post in the related issue. You could post either a text description or a rough sketch, and the designers working on Project Browser will be happy to try and produce it.

As you say, the logo needs to be called logo.png. This filename is used in the merge request.

Yes, the file needs to be called logo.png. That filename is is used in the MR.

Yes, you're correct, the file needs to be called logo.png. The file in the MR uses that name.

rachel_norfolk → credited lostcarpark → .

Thanks Kristen,

Sorry I forgot to update here...

lostcarpark → created an issue.

By default, tests are allocated 2 CPU cores. As the Project Browser tests are quite intensive, this is giving inadequate resources.

Increasing the value of KUBERNETES_CPU_REQUEST to 16 will hopefully give the tests sufficient resources.

However, while I had several successful test runs, I did have one failure of the FunctionalJavascript tests, even with this change.

I'm hopeful that this will improve reliability, but I think we also need to look at moving some of the tests to Nightwatch.

Summary of issues fixed:

• src/Plugin/ProjectBrowserSource/MockDrupalDotOrg.php
  • getWarnings() did not have a parameter type - Set to array
• src/Plugin/ProjectBrowserSourceBase.php
  • Constructor did nothing but call parent constructor with same parameters, so was redundant - Deleted
• src/Routing/ProjectBrowserRoutes.php
  • Use statements were not alphabetic
• src/ProjectBrowserFixtureHelper.php
  • Use statements were not alphabetic
• src/ProjectBrowserServiceProvider.php
  • Use statements were not alphabetic
• tests/modules/project_browser_test/src/DrupalOrgClientMiddleware.php
  • An array element had "," where it should have had "=>", making the array index look like an extra element
• tests/modules/project_browser_test/project_browser_test.module
  • Module file should prefix function names with module name, but was prefixing with "project_browser" instead of "project_browser_test"
• tests/src/Functional/DisableAddNewModuleTest.php
  • Use statements were not alphabetic
• tests/src/Functional/InstallerControllerTest.php
  • Use statements were not alphabetic
• tests/src/Kernel/PackageNotInstalledValidatorTest.php
  • t() function was being called with variables instead of string literals. Passing variables to t() is discouraged. As the variables were fixed valies, I just moved the literals into the t() function calls

Also made PHPCS mandatory so any failures will cause the pipeline to fail.

lostcarpark → created an issue.

All spelling checks now passing.

lostcarpark → created an issue.

lostcarpark → changed the visibility of the branch 3420552-xpath-checks to hidden.

I have disabled the parallel running in .gitlab-ci.yml. I have run 3 times in a row without any failures. Probably should try a few more to be sure, but that seems good enough to move to Needs Review.

Should we leave the parallel section commented for now, on the assumption that we intend to restore it later, or delete it?

lostcarpark → changed the visibility of the branch 3420552-functionaljavascript-tests-can to hidden.

Provided comments for return types and classes.

Noticed a minor wording mistake in the function comment for ProjectBrowserSourceInterface::getDevelopmentOptions, so fixed that as well.

lostcarpark → created an issue.

I think the issues should all be fixed now.

I have rebased.

Brought in a couple of CSS changes I wasn't expecting. I think they were from other commits.

lostcarpark → created an issue.