Account created on 4 August 2008, almost 16 years ago
#

Merge Requests

More

Recent comments

🇮🇪Ireland lostcarpark

Oh yes, I'd forgotten that comment. +1 for option 1, the rounded version.

🇮🇪Ireland lostcarpark

I've added a very simple Nightwatch test to test keyboard control of the category drop-down. I need to figure out how to make it actually run in the CI, and it will need additional keyboard test cases added.

🇮🇪Ireland lostcarpark

I have tested manually in DrupalPod, and verified the expected projects are now appearing...

All tests are passing, so this looks good to me.

Moving to RTBC.

Thanks for the quick fix.

🇮🇪Ireland lostcarpark

I believe this should be fixed by 🐛 SID 2776211 contains div element Needs review .

I'm updating to "maintainer needs more info", and would appreciate if anyone could retest.

Once I'm happy this is no longer an issue I'll move to "Closed (outdated)".

🇮🇪Ireland lostcarpark

Thank you for your work on this.

The tests are passing now for current and next_major.

Unfortunately, they are failing for previous_minor and previous_major. This is because renderInIsolation was only introduced in Drupal 10.3.

For previous_minor, we could use DeprecationHelper as follows:

$output = DeprecationHelper::backwardsCompatibleCall(
    currentVersion: \Drupal::VERSION,
    deprecatedVersion: '10.3',
    currentCallable: fn() => \Drupal::service('renderer')->renderInIsolation($build),
    deprecatedCallable: fn() => \Drupal::service('renderer')->renderPlain($build),
);

Unfortunately, DeprecationHelper was only introduced in 10.1, so for the moment at least, we'll need additional measures to keep support for Drupal 9.5. I think we need to support this at least while automated tests run against it. I think we could handle all supported versions with:

if (version_compare(\Drupal::VERSION, '10.1.0') >= 0) {
  // Drupal 10.1 or later, use DeprecationHelper.
  $output = DeprecationHelper::backwardsCompatibleCall(
    currentVersion: \Drupal::VERSION,
    deprecatedVersion: '10.3',
    currentCallable: fn() => \Drupal::service('renderer')->renderInIsolation($build),
    deprecatedCallable: fn() => \Drupal::service('renderer')->renderPlain($build),
  );
}
else {
  // Remove when we drop support for < Drupal 10.1.
  // @phpstan-ignore-next-line
  $output = \Drupal::service('renderer')->renderPlain($build);
}

Please update the fix and see if tests will pass.

🇮🇪Ireland lostcarpark

Confirming logo meets Project Browser requirements.

Logo is PNG image, dimensions are 512x512, file size is under 10K. Logo looks good and is consistent with other JQuery UI logos.

Moving to RTBC.

🇮🇪Ireland lostcarpark

As per #13, confirming logo adheres to Project Browser requirements.

Moving to RTBC.

🇮🇪Ireland lostcarpark

As per #12, confirming logo meets Project Browser requirements.

Moving to RTBC,

🇮🇪Ireland lostcarpark

Confirming logo meets Project Browser requirements.

Image dimensions are 512x512. File is PNG image, and file size is under 10K. Request to make background transparent has been completed.

Moving to RTBC.

🇮🇪Ireland lostcarpark

Confirming proposed logo meets Project Browser requirements.

Logo is PNG image, dimensions are 512x512, file size is under 10K. Also request for transparent background has been completed.

Marking RTBC.

🇮🇪Ireland lostcarpark

Reviewed and confirmed image dimensions are 512x512, file is correct format and under 10K, and background now transparent.

Ready to mark RTBC, but will wait for @Kristen to move to correct project.

🇮🇪Ireland lostcarpark

Confirming logo is a PNG file, dimensions are 512x512 pixels, file is under 10K, and now has transparent.

Happy to mark RTBC, but if you're moving the issue into the project queue, I'll wait for that.

🇮🇪Ireland lostcarpark

I definitely prefer the rounded version to the sharp version. I'm not clear on what's changed between @urvashi_vora's logo and @AaronDeutsch's version. I can see some square corners on the lock have been rounded off. Anything else?

🇮🇪Ireland lostcarpark

I've carried out manual testing in Drupalpod, and confirm the Add module option is no longer available, so confirm RTBC.

@dww I was unsure whether this is considered a new feature, or removal of a deprecated one. If it can go in 11.1, that would imply it's considered a new feature. However, if it's removal deprecated code removal, presumably the feature would have to be marked deprecated in a release (possibly 11.1), and the actual removal would come in 12.0. I would probably consider 11.1 preferable to waiting for 12.

I'd also wonder can this also be backported into 10.4, or does it have to be 11 only?

🇮🇪Ireland lostcarpark

This is great news!

We have functionality to hide this menu and route in Project Browser, so we can take that out when this gets merged.

Is this expected to make it into D11? I would assume it would need a major release.

🇮🇪Ireland lostcarpark

I can see the logic that recipes are "applied" to a site, rather than installed. However, I think for a novice site builder, it's not very clear what "Apply" means when it's on a button. When I see an "Apply" button on a website, it implies "make a request", as in "Apply for a loan", "Apply to join a group", "Apply for job".

I think this can be solved. Perhaps something like "Apply to site" would be clearer?

I to like the idea of making the button reflect the type of item being browsed, so in principle, I think it's a good change.

But I think we need to think carefully about our choice of button labels.

One of a new users' first interactions with Drupal will be a mini browser of recipes, and in that instance will "Install" be clear, since from their point of view, they're already in the middle of the install?

🇮🇪Ireland lostcarpark

I do actually like @urvashi_vora's logo in the Proposed Resolution, but there is a lot of whitespace around the edges. I feel it needs the logo part blown up to fill the space.

🇮🇪Ireland lostcarpark

I wonder does this issue need further review. There seem to have been several options proposed, but no actual discussion on them. Not sure was there any reasoning in selecting the logo in the Proposed Resolution.

Setting back to Needs Review. Please move back to RTBC if this is settled.

🇮🇪Ireland lostcarpark

Just noting that all tests are now passing.

Working on adding test for keyboard navigation.

Still need to rebase for latest changes to 2.0.x branch.

🇮🇪Ireland lostcarpark

I'm not sure that we want to go the whole "shopping cart" route. I think "App store" is more appropriate.

I like the Linux analogy. You ask it to install a package, and it finds all the dependencies it requires, and asks you to confirm.

The actual mechanism would behave slightly differently. Linux figures out what it's going to do, then asks you before it attempts it. Package manager applies the change to the staging area, then lets you confirm before applying to the site.

Automatic Updates already has this, so hopefully we can do something similar. I don't doubt @phenaproxima that it will be a big refactor, but at least we have a model to follow.

I have ideas for post-install, but I would suggest we focus on confirmation during install for this issue.

🇮🇪Ireland lostcarpark

Latest changes to the parent issue may cover this. Clicking outside the category dropdown, or tabbing off it, will cause the drop-down to close.

🇮🇪Ireland lostcarpark

One thing to bear in mind is the buttons could make things more difficult for keyboard users. Some thought would need to be given to make sure they aren't disadvantaged.

🇮🇪Ireland lostcarpark

Added Tab and Shift+Tab keyboard behaviour.

Now when pressing Tab withing the open drop-down list, it will navigate to the next filter (Security Coverage). Pressing Shift+Tab will navigate to the previous one (the search bar).

Currently the previous and next destinations are hard coded. It would be nice to make that more generic, in case the available filters vary between browser tabs, but I'm not sure how to make that work.

I feel this helps to make the drop-down list feel like a single control, rather than a collection of separate checkboxes, and makes it behave in a consistent way with other drop-down lists.

🇮🇪Ireland lostcarpark

I've now removed the chips for non-category filters as suggested in #49.

I also have it showing the selected categories (when there are categories selected), replacing the "Select categories" in the categories box.

At present the category box gets taller as you select more categories. I thought this would look unsightly, but I think it looks surprisingly okay.

We possibly should put a limit on how tall it can get. I can think of several approaches we could take:

  1. Just let it grow to show all categories, since most people will only pick a couple.
  2. Put all the categories in the box, but limit the height with max-height
  3. limit the number of categories shown, and have a "+X" at the end to indicate the number of unshown ones
  4. Show the category name when one category is selected, and "X categories selected" if more than one
  5. Only show "X categories selected", never the names.
🇮🇪Ireland lostcarpark

I have fixed some issues for both mouse and keyboard navigation.

Selecting/deselecting a category with either keyboard or mouse will now keep the drop-down open, and keep the focus on the same checkbox.

Clicking outside the drop-down, or tabbing off the last checkbox item will close the drop-down.

I've started making the selected categories display in the closed category box, as would be the case for a standard drop-down. At present I only have it displaying the ID, when obviously we want the category titles displayed. From Chris's comment #49, we could make it just show the count of selected categories.

🇮🇪Ireland lostcarpark

Once again, I love the design. I think all of your logos look great.

As with 📌 Create logo for jQuery UI Selectable for Project Browser Initiative Needs work , please make the background transparent.

🇮🇪Ireland lostcarpark

I love the overall design!

I'm not sure the hollowed out UI logo is the right effect, as normally a button would be a solid item with a drop shadow. I would suggest making the interior solid white, and take away the shadow showing through.

And as with 📌 Create logo for jQuery UI Selectable for Project Browser Initiative Needs work , please make the background transparent (but not the interior of the UI).

Hope that makes sense!

🇮🇪Ireland lostcarpark

Another great design.

As with 📌 Create logo for jQuery UI Selectable for Project Browser Initiative Needs work , this just needs a transparent background.

🇮🇪Ireland lostcarpark

One other thing to consider is the dashed grey line. This should stand out on a light or a dark background, but could get lost on a mid-grey theme. I don't know how likely we are to get a mid-grey admin theme, since generally an admin theme aims to maximize readability, should be high contrast. Just something to be aware of.

🇮🇪Ireland lostcarpark

I love the logo.

There's just one very small thing. The background is solid white.

That will look fine in Claro, but in other themes such as Gin, this could look sub-optimal. In light mode, the Gin theme uses an off-white colour, so the logo would have a pure white box looking slightly off from the surrounding area. In dark mode the logo will stand out in a white box on a dark background.

I would recommend replacing the white background with a transparent background, so the logo will look good on all backgrounds.

I hope that will be a small change and can the move forward with the logo.

🇮🇪Ireland lostcarpark

@chrisfromredfin, agree with removing the chips for the other filters.

I'd suggest we go further. When closed the category drop-down always says "Select categories". When there's nothing selected, that make sense. However, when categories are selected, I think it should show the categories in the closed drop-down box.

Then, do we need the chips at all?

🇮🇪Ireland lostcarpark

@rkoller, I think it was you that had an issue with the page jumping when you press space to open categories in Chrome/Safari. I had only tested in Firefox. I've fixed that issue and tested in Chrome (I don't have Safari).

Still working on making it actually select categories.

🇮🇪Ireland lostcarpark

As per comment #32, the order of the drop-downs has been changed into a more logical order.

However, the active filters are shown below it the reverse of that order. E.g. "Active - Maintained - Covered by security policy - Content Display - E-Commerce.

For me it makes no sense to show the filters in the reverse order of the drop-downs.

We should either:

  1. Always display in the same order as the dropdowns, so "Categories, Security Policy, Maintenance, Development
  2. Add the filters in the order they are selected, so the most recently added filter will always be on the right. If a filter is removed and readded, it would move to the end of the list.

I don't have a strong preference between these, but we should choose one and implement consistently.

🇮🇪Ireland lostcarpark

I have been tinkering with keyboard control. I have got it so that pressing "space" or "alt+down" or "alt+up" opens the drop-down, and moves the focus to the first checkbox.

Up/down arrows will then move between checkboxes in the list.

However, I'm having trouble toggling the checkbox with the keyboard.

🇮🇪Ireland lostcarpark

@rkoller Are you able to run the current 2.0.x branch? I found I needed to update a few things with Composer and do a drush updb to get things working.

🇮🇪Ireland lostcarpark

I have taken the first step of adding a keyDown handler that opens/closes the dropdown when pressing "space" or "alt+down" or "alt+up", which matches the behaviour of the other dropdowns.

However, when it expands, the focus is still on the parent control. I think it should move to a child control (the first checkbox?) to start navigation within the drop-down.

Also need a keyboard handler for navigating between checkboxes withing the drop-down.

It might also be worth asking is there a prebuilt checkbox control we could use that has keyboard handling built in?

🇮🇪Ireland lostcarpark

Also when category drop-down is opened with the mouse, I can navigate the checkboxes with tab/shift+tab, but not with up/down arrows.

I think most users would expect to navigate drop-downs with up/down, and for tab to close the drop-down and move focus to the next form field.

🇮🇪Ireland lostcarpark

I have tested this change and it works well for me, and the filter drop-downs are in the right order.

I have encountered a couple of issues:

  1. I can't seem to open the categories drop-down with the keyboard. The other filters can be opened by pressing "space", but that doesn't work for categories. I feel we need an intuative way of making it keyboard accessible.
  2. You can click the checkbox on a category, or the category name to select/deselect. However, clicking the category text seems to have a very tight "target" area, and clicking just outside the text makes the drop-down disappear without selecting. As there is a bit of a gap between items, I think it would be easy for users to miss the target, and could be confused about why the category wasn't selected. I think this could be fixed by adding a small amount of padding around the category text.
🇮🇪Ireland lostcarpark

As test is passing in pipeline, have merged and marking as fixed.

🇮🇪Ireland lostcarpark

Test moved and now runs in pipeline. Passing for all tested Drupal versions.

🇮🇪Ireland lostcarpark

I think CSpell fixes are uncontroversial, so merging this change.

🇮🇪Ireland lostcarpark

Thanks @nilesh.addweb, change looks good. Merging.

🇮🇪Ireland lostcarpark

Added CSpell fixes.

CSpell now passes on GitLab CI.

Moving to Needs Review.

🇮🇪Ireland lostcarpark

Thank you @sarwan_verma and @abhiyanshu for your work on this.

Merged change and marking Fixed.

🇮🇪Ireland lostcarpark

@sarwan_verma Thank you for your MR.

I have made a small change to add a Deprecation Helper to maintain compatibility with Drupal 10.2.

🇮🇪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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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

Moving to RTBC.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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

🇮🇪Ireland lostcarpark

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

🇮🇪Ireland lostcarpark

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

Thanks for this feature.

🇮🇪Ireland lostcarpark

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!

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

🇮🇪Ireland lostcarpark

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.

Production build 0.69.0 2024