🇺🇸United States @chrisfromredfin

Actually, you did! :) Added a related here for cross-reference purposes.

chrisfromredfin → created an issue.

chrisfromredfin → created an issue.

chrisfromredfin → created an issue.

This is an interesting concept. It also advances us toward potentially replacing the existing "List" tab of the module page now.

Removing the "array_filter()" at the top of getCoreModules() gets us really close as a proof of concept. But things to be decided... for modules that ARE part of drupal.org, how could we detect that and fetch the correct relevant data for them (security coverage, number of installs, etc). We could still use the categories that are present on the existing List page, but they would then be _different_ from if you were browsing on the drupalorg_jsonapi (something brought up frequently in early UI discussions - i.e. can we make all those categories MATCH?)

Also, doing this scan might bring up custom modules that don't have anything to do with drupal.org.

One possible answer is to stash some of this kind of metadata in the .info.yml. For drupal.org projects, maybe that happens with packaging script - like writing out the category or something, but that may open up a can of worms also?

If anyone has any thoughts on these, I'd be interested in hearing them. :)

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

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

chrisfromredfin → created an issue.

This is working well for general keyboard nav.

Let's open a follow-up for anything related to VoiceOver that is still not ideal. In general I much prefer small, actionable, discrete things rather than "fix all a11y issues with __X__" since they're easier to review & go in, so let's do that!

Thank you EVERYONE for your work with addressing, testing, and thinking to help be inclusive for ALL users of Project Browser!

NOICE

when things rot, they should be carved out! :)

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

When a problem comes along, you must ship it!

Ralf, I've noted your concerns in a more-related issue, and am closing this one as outdated since we can't leak any more.

I asked Ralf to comment his concerns about "everything-across-the-top" here, but I believe he added it here: https://www.drupal.org/project/project_browser/issues/3457966#comment-15... 🐛 Filter (categories) selection is leaking over into other sources Active

made it!

modals negate this issue

✅

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

Excellent!

Wonderful work, y'all. This surfaced a follow-up about "caching" results, which include errors. But that's a bigger can of worms.

A lot of that can be fixed by making (a) the recipes/recommended plugin first in the order in config and (b) making "browse" the default action of the "Extend" menu. I'm sure you can do (a), but without code I'm not sure you can do (b).

I am thinking in terms of the overall architecture of the tabs now, I find it a confusing mess in Drupal CMS right now:

1. Recommended, first but not selected-by-default
2. List - ok there's a verb
3. Browse - ok, a verb, must be another version than list
4. Update - ok, a verb
5. Update Extensions - ok, umm, but why is that separate?
6. Uninstall - ok, a verb

IMHO, this would be a lot better with List, Browse, Update, Uninstall - with two browsers under browse, and two updates under update.

Even better might be:
- Install (Browse recipes, Browse modules, List (or even "Available"))
- Update (Update, Update Extensions)
- Uninstall

But that's a much larger / even core issue that cannot be decided. I'm still not clear on why there isn't like a "Drupal CMS" glue-type module for various tweaks and code that might make the product better. ¯\_(ツ)_/¯

Well look at that, we're green again! 😜

ship it dot com

I'm gonna take this for now because I want it in, but I think the test should have been more generic and/or at a minimum doesn't really belong in the TestDrupalOrgJsonApi - really a test that "if any backend gives an error with its response, that error is displayed" - but this test DOES do that, so I think refactoring in a follow-up is best.

But also, I'm speaking a little bit out of my pay grade, so I welcome any response to this idea. :)

#19 links to a shared Google Doc with a revised, simplified mockup.

I would not be opposed to a new issue being opened that brings in elements of the original wireframe back in, but I would want it to be run through usability with our target audience. I'm not here to snub open source developers, I am one, but I every addition should be intentional.

But either way, not in this issue. :)

Good work so far, but we need to make this its own test. I think you could start with the test you injected this into and copy/paste as a starting point, but then you need to do a search that gets only one result.

Here's some completely untested code to get you started:

/**
   * Tests formatting plural for results.
   */
  public function testSearchForSpecialChar(): void {

  // Load browser.
    $this->drupalGet('admin/modules/browse');
// Check for plural.
    $this->svelteInitHelper('text', '10 Results');

    // Tests for the presence of search bar placeholder text.
    $search_field = $this->getSession()->getPage()->find('css', '#pb-text');

    // Fill in the search field.
    $this->inputSearchField('', TRUE);
    $this->inputSearchField('Vitamin', TRUE);
    $this->assertSession()->waitForElementVisible('css', ".search__search-submit")->click();

    // Test for result count text (singular).
    $this->assertEquals('1 Result', $this->getElementText('.pb-search-results'));
}

On the latest 2.0.x as of right now (46ab2a2) - I can go to recipes tab, apply a recipe, it shows "installed ✔️" - then I reload the page, and it still says installed.

In an IDEAL world, it would be nice to actually record a LOG of when recipes were applied and be able to show the user that it has already been applied at such and such time/date. However, I'm not yet sure what the UI for that would be.

I THINK my personal belief is that given the option of what we have now (disallowing reinstall) or allowing users to re-apply (and they simply must track themselves if they've applied a recipe before or not), I THINK I would choose the latter. But I'm happy to be outvoted by other UX folks' opinions.

I think that Drush has its own way to mock those things, actually - which would also mean we can remove the package.

@see
Drush Test Traits - https://www.drush.org/13.x/contribute/unish/#drush-test-traits
-and-
https://git.drupalcode.org/project/devel/blob/8.x-2.x/tests/src/Function...

...maybe?

Reviewed, manually tested. Fixed.

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

This is happening only in Drupal CMS I believe, and though ugly, it goes away because Drupal CMS will be only allowing one-at-a-time installations.

It will also be fixed with the new UI in 📌 Change UI for the install queue to match bulk operations Active , so effort on that issue is better.

(Also, you'll notice I rebased this but it's failing a bunch of tests now?? Was that me? 😬)

So on principle this looks great to me, however - it really needs a test. I assume a FunctionalJavascript so we can test that the message is displayed on the front-end using JS... but I have no idea how to fake/mock the endpoint result.

But we need to test that if we create a results page with an error, that error is displayed.

I would love to (could be in the future/follow-up) add support for warnings in addition to errors (so we could notify of an upcoming deprecation).

But I think the approach is right.

This is a great net-negative, and more secure. And hits up on some work we needed to do with the DA. Huge win. 💪

one small issue from manual testing -

on 2.0.x
modal - icon & number appear
card view - just number appears
list view - icon & number appear

in this MR
modal - icon & number (good)
card view - just number appears (good)
list view - just icon appears (bad)

I don't think this needs a test in this MR. Ideally there would be one since we obviously caused a regression that wasn't caught, but we have other open issues about the icons appearance, etc - and I will do them there.

@ressa, yes - let's do that.

Yeah, this rocks.

NW for making configurable

This diff should get us toward a selector we can use to style per-source. Stuffing it here before I run off.

diff --git a/project_browser.module b/project_browser.module
index a823a55..5fe7705 100644
--- a/project_browser.module
+++ b/project_browser.module
@@ -38,7 +38,7 @@ function project_browser_help($route_name, RouteMatchInterface $route_match) {
 function project_browser_theme() {
   return [
     'project_browser_main_app' => [
-      'variables' => [],
+      'variables' => ['source_plugin' => NULL],
     ],
   ];
 }
@@ -59,3 +59,15 @@ function project_browser_project_browser_source_info_alter(array &$definitions):
     ];
   }
 }
+/**
+ * Implements template_preprocess_HOOK().
+ *
+ * @param mixed $variables
+ * @return void
+ */
+function project_browser_preprocess_project_browser_main_app(&$variables) {
+  // Pass the current route's "source" parameter as the source_plugin var.
+  $route_match = \Drupal::service('current_route_match');
+  $param = $route_match->getParameter('source') ?? NULL;
+  $variables['source_plugin'] = $param;
+}
diff --git a/templates/project-browser-main-app.html.twig b/templates/project-browser-main-app.html.twig
index f3768f4..b94487a 100644
--- a/templates/project-browser-main-app.html.twig
+++ b/templates/project-browser-main-app.html.twig
@@ -11,4 +11,4 @@
      
   </div>
 </div>
-<div id="project-browser"></div>
+<div id="project-browser" data-pb-source="{{ source_plugin|clean_class }}"></div>

See https://drupal.slack.com/archives/C01UHB4QG12/p1736163780116629

I've rebased to latest and then manually tested. It seems now though that the icon is gone on the Grid view.

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

Can I confirm that someone has browser-tested across many things? The reporter was most likely on Linux/Firefox from his screenshot. But we need to check Firefox, Safari, Chrome, etc.

Leslie has just suggested something good here. Can we split the difference and simplify by adding a ? or [i] icon next to the category label, and it opens a modal with all the terms & descriptions? This way you still don't have to leave the site and it loads fast. We might be able to then later use a anchor/fragment to link directly to one term/description.

I think in general I would rather a user opt-in to getting the definition rather than it just showing up on hover or focus. As it stands now the description might cover the next term and the user didn't even need to know the definition.

Thank you!

Rebased this and tested and it works great. Tests pass. However, I'm unsure about the use / need for php-mock. Doesn't core have a supported way to do mocks without this library? OR, does core use this library and we just don't yet?

Looks better now!

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

Duplicate of 🐛 The project title's line height is a bit off. Active - I created this not knowing the other existed. Both are fine fixes but we prefered the 1.75 line height. ;)

This feels good to remove cruft from a feature we no longer have! 💪

At mobile, it seems to be still the single blue button - we need to also match mobile styles like we see on /admin/content, for example.

Updating IS to show what really needs to happen here.

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

Thanks Naren for bringing up those points, which are important. So, this is a good workaround for now in order to get Drupal CMS out the door.

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

I think it is more distracting to switch sources and have an inconsistent UI. I would prefer to leave a default logo there.

Hard to manually test visually-hidden things, but we got there!

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

Thanks! Merged as is, we really need to figure out what's up with Nightwatch tho... at least for now until we clean up the tests en masse.

Tests broken already, fixing in related issue. Manually tested and reviewed.

This is a great step in the direction of introducing user interaction during Activation of a project. Love it! And it makes recipes SO powerful.

chrisfromredfin → created an issue.

Checked on 11.0 and got an error relative to the Analytics recipe, which is expected! So, good there anyway,

Went to 11.1 and got all the way through the input, and then the application, then looks like we missed new requirements from the other issue (project_browser.browse now requires a source param).

Acknowledging that we're really starting to shift slop around in here, it's still nonetheless a step forward.

We need to really take a cleanup pass in here, per Adam's thoughts in #15. We will do that in follow-ups. The good news is this will result in far SIMPLER and MORE MAINTAINABLE front-end code.

At least this bug - hopefully it's not a can of worms:

Start on the "Browse" (modules) tab. Pick a category (ex.g. "Access Control"). Click to the Recipes tab. Recipes shows a flash with all of the included filters, then hides the filters, leaving recipes to show 0 results. (Full page reload doesn't help. Must be session storage.)

(Of note is that doing this and switching over to the random data plugin also shows 1 category there, but 'undefined' since I'm sure the actual category doesn't match.)

Hopefully there's a baby-step fix here but I think the separate nature of these changes where they're on separate tabs makes sense to really decouple ANY kind of sharing between filters. Even if they have the same filters, don't carry them over - make the user pick. Even if keyword is shared between the two, it now makes more sense to really decouple all those... each backend plugin gets its own and never the twain shall meet.

Hopefully, like I said, there's a lightweight approach we can take here, initially, for the benefit of unblocking Drupal CMS. Or maybe ripping out anything trying to do this IS the easiest fix.

Same on 11.0.9 and 11.1.0.

Giving this a review & test per a discussion today with @phenaproxima

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

Additional info, I tried to adjust the recipe.yml to something valid, and though I now get the prompt for my tag ID, it fails after submitting that with:

The website encountered an unexpected error. Try again later.

LogicException: Input values cannot be changed once they have been set. in Drupal\Core\Recipe\InputConfigurator->collectAll() (line 141 of core/lib/Drupal/Core/Recipe/InputConfigurator.php).
Drupal\Core\Recipe\InputConfigurator->collectAll() (Line: 139)
Drupal\project_browser\Form\RecipeForm->setRecipeInput() (Line: 112)
Drupal\project_browser\Form\RecipeForm->validateRecipeInput() (Line: 58)
Drupal\project_browser\Form\RecipeForm->validateForm()
...

Though these may be issues in the recipe(s), I wonder if we should handle them in a slightly better way.

Manually tested.

There was an issue on 11.0.4(?) that Adam already fixed, regarding missing property $input.

I think bumped my Drupal version to 11.1.0 and tested with the default website contact feedback form recipe, which worked great - prompting me for an email address, applying steps using a batch API progress meter, and dropped me right back where I expected.

I think tried with drupal/drupal_cms_analytics recipe, but I never got to the screen where I needed to enter my analytics ID. Instead I got to this URL https://pb11.ddev.site/admin/modules/browse/recipe-input?recipe=/var/www... with an error:

The website encountered an unexpected error. Try again later.

Symfony\Component\Validator\Exception\ValidationFailedException: Object(Drupal\Core\TypedData\Plugin\DataType\StringData): This value should not be blank. (code c1051bb4-d103-4f74-8988-acbcafc7fdc3) in Drupal\Core\Recipe\InputConfigurator->collectAll() (line 162 of core/lib/Drupal/Core/Recipe/InputConfigurator.php).
Drupal\project_browser\Form\RecipeForm->buildRecipeInputForm() (Line: 41)
...yadda yadda yadda...

I think doing this breaks a somewhat-fundamental (albeit poor) assumption that this would always be a bit of a SPA. So, I think we need to introduce state handling or something into the front-end to make sure that we're clearing previously-stored tab state if switching routes, otherwise you get to the "Recipes" route and still see previous iterations of the "Modules" tab.

VIDEO DEMO: https://www.drupal.org/files/issues/2024-12-18/Monosnap%20screencast%202... →

Also, put Recipes _after_ Browse in the tab order.

I'll open a follow-up to see if we want to change "Browse" to "Modules" or change both to "Browse Modules" and "Browse Recipes" - but those shouldn't hold anything.

chrisfromredfin → created an issue.

Related to this, does project_browser/package_manager respect a minimum stabiility flag?

Yes, PM does the equivalent of `composer require drupal/some_project` so that's why we don't really know what package will be installed, because it specifically depends on Minimum Stability.

What is the minimum-stability set to in drupal/recommended-project? In institutional settings, I imagine a power user would set minimum-stability to their comfort level and users using PB would just click install, not caring about module versions. That's very much in our user world - I would be reticent to add more additional info to cards, and would be more inclined to support pre-or-post messaging to the user per ✨ Add confirmation page when installing module Active or ✨ Show users what's changed on confirmation screen Active .

A pre-or-post confirmation screen also may relate to ✨ Create a way to accept recipe input when applying one with Project Browser Active .

chrisfromredfin → created an issue.

chrisfromredfin → created an issue.

I think this can work. We have support to show Drupal messages in PB already, afaik (for example if there is a verification problem with Package Manager), so I believe the backend can utilize this to show an info/error/warning message as appropriate.

good

Found some additional issues with opening in a new window - fixed this one, and another place in this file.

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

There will be more a11y fixes coming, but I aim to try to keep them as small and narrowly scoped as possible.

teamwork makes the dream work.

Thanks for the reviews, Naren!

Thanks!

This is much more semantic, and also will help with tests!

Per Ben in Slack:

Decoding should happen on the Back End, in as early a stage as possible after the data is received. The one exception is if there's some reason the non-decoded should be available

I think the approach we need, then, should actually be in the ProjectBrowserEndpointController.php before we return JSON to the Project Browser, or maybe the EnabledSourceHandler right when we get the response and before we put it in the KeyValueStore. Possibly we need to do this for each field that could contain HTML inside the individual sources.

I think we need to push the idea of "project" more universally, both with "Project Browser" and in the Drupal world as well. We've always had this distinction (a project, for example, can be made up of many modules, actually). And we really are working at the project level.

This is a big step forward, so I am going to send this as is. We can revisit later if there is demonstrated user confusion.

Having trouble with the UX of changing tabs where each tab can have a different sort order. Can you make a recommendation about the UI here?

https://www.drupal.org/project/project_browser/issues/3481652#comment-15... 🐛 SortBy filter is conflicting after switching between different tabs Active

So given that #3 is generally the preferred behavior, the open question is still what should we do in this situation?

(1) Go to source "D", sort by Z-A.
(2) Jump back to source "A" which has no Z-A.
(3) So it sets source "A" to sort by its default.

CURRENTLY it's also setting ALL sources to sort by their defaults, too, so
(4) Jump back to "D" and it's no longer Z-A

Should we instead keep D's sort to Z-A, effectively meaning we're storing the active sort per source? But if that's the case, what would happen if I:

(1) go to source "D" and sort Z-A
(2) go to source "A" which has no Z-A so it sorts by Relevancy
(3) change source "A" to "date"
(3) go back to "D"
- should be it still on Z-A? Or should it be "date" (if available) or should it be its default?

chrisfromredfin → created an issue.

I have had a vision for a pre-or-post confirmation in the works, it would need some effort to get it implemented, and we'd need to know what's possible from Package Manager in order to display the confirmation, ex.g. It also may not be possible until half-way through the process before PM re-applies to the site.

Things that make this hard:

(1) the security coverage IS itself a project-level flag on Drupal.org - whether or not that project is opted in. The fact that you are getting an uncovered 3x is more about the governance rules of what gets actual reviews (green releases only)

(2) whether or not. you get the "green" or "yellow" can actually depend on your composer config - whether you allow beta releases, for example.

I'm not sure if we can just educate users around this or if there's something more drastic that might need to happen. I'm not really convinced it's a true security issue, it feels like a UX issue.

One concern to bring up with this approach is that code is tied to a "release" not a "project" - so as a gross example, version 1.1 of a recipe can be a "kit" and then 1.2 can be a "foundational" - not that that makes sense, but it is possible.

So, if you're trying to mix and match things in code and things on d.o (as you are with the "sub-type" versus the "tags") I might encourage you to put them all in the same place (d.o)

Also, I would encourage you to look at the newly-revised categories we came up with for modules to see how close they might come to the categories you would want for your recipes.

I am unsure if we should be fixing this in the Svelte frontend or in the Middleware. I will ping Ben to get some opinions. Also, it's entirely possible we want to do both. Third, I'm not sure if we want to introduce a front-end dependency on HE or not.

I need more info.

All changes were to tests, code looks good. Tests are passing.

I have one small nit (filename) and then this needs manual testing but then is probably ready to go.