Portland, Maine
Account created on 12 August 2006, almost 18 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

OK, after some more digging it seems the pager just flat-out doesn't work correctly for anything except the first tab that's enabled. Depending on how you order it, that one will work. So by default, paging doesn't work on the recipes plugin. But, if you make Recipes the first in the order, it will work (and then won't work for plugins 2 or 3).

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I see the value here in asking an LLM "what modules should I use to accomplish X?" - and I think it would be a great thing to bolt on to PB someday, maybe.

Note that I use AI/ML (an actual custom clustering model - not just "ask the LLM") to actually feed the descriptions of the Top 100 modules into an LLM and have it come up with word clusters as one of the ways we came up with our new list of 19 categories. πŸ˜‰

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Thanks, @gslexie! I tend to agree that would be the preferred path forward.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

It's certainly non-trivial to implement the Combine filter for APath/JSONPath, but I don't think impossible. It seems like we could use regex to search the multiple fields selected in the views filter:

$..[?(@.field1 =~ /.*someword.*/ || @.field2 =~ /.*someword.*/)]

After a little more digging, it seems that at a minimum, placeholder() and addWhereExpression() would need to be implemented. The placeholders would define where to substitute what in the addWhereExpression, which could ultimately hopefully get to specifying the correct APath.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Yes, it would seem that in order for Views Json Source to support combined fields filtering, it the ViewsJsonQuery class needs to implement the ::placeholder() method.

This method is invoked by Drupal core's views HandlerBase for combined fields filters.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

chrisfromredfin β†’ changed the visibility of the branch 3455152-remove-originurl-constant to hidden.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

many folks doing great work on this now...

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

We should wait for changes here since the whole detail page is being worked on in πŸ“Œ Update project detail page layout & elements Needs work

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Please re-check this on latest 2.0.x. I think I was able to see the behavior only the very first time I loaded it after an install, but then never again.

Also, I think paging for Recipes is completely broken, actually - we may not be storing pager settings correctly, or not resetting them when switching tabs (for example I can switch from one source to the other and the pager still thinks I'm on page 3 - and changing pages doesn't change which recipes it's showing me).

Can we get some additional testing on the 2.0.x and an update of this issue summary with the needed fixes?

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

This is being worked on actively (literally, right next to me right now πŸ˜†) by Bernardo in πŸ“Œ Update project detail page layout & elements Needs work

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Yes, we'd also need something on the project object in the frontend, sourced by the backend, to show the language rather than hardcoding 'Install' here:
https://git.drupalcode.org/project/project_browser/-/blob/2.0.x/sveltejs...

ActivatorInterface.php would need to require a new method, something like getActionText(), which would return at least two states of text (one for if the project is already available, and one for if it is not yet available), for example "Add & Install" vs just "Install" - for recipes, they may both just be "Apply" (we're considering just "Install" regardless for PB anyway for modules - because we're not sure if communicating this to the user is even helpful).

While we're at it, we may need to also do a "View Commands" text version as well... if we're building it to be flexible.

Will run this API change by @phenaproxima, but it's clear this is not a novice issue any more.. πŸ˜†

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Chandansha - can confirm that in this MR, only the bundle.js has any changes. I would expect to see some changes in the Svelte source as well.

I'm also unsure how it was handled in the source, but I think generally speaking if we want to change this copy, we shouldn't just be doing it with an "if" in the Svelte; I think this is a non-trivial change to allow the source plugin to provide its own language for its action button.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I opened πŸ“Œ Only show number of installs if non-zero Active to deal with the "installs" language for now :)

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

since this was worked on and tested and RTBC'd, I'll ship it!

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

actually this should block alpha

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

@phenaproxima is going to look at this soon (next day or two)

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

In InstallerController.php, it seems like Installing is hard-coded to only support drupalorg_mockapi:

    // @todo Expand to support other plugins in https://drupal.org/i/3312354.
    $source = $this->enabledSourceHandler->getCurrentSources()['drupalorg_mockapi'] ?? NULL;
    if ($source === NULL) {
      return new JsonResponse(['message' => "Cannot download $project->id from any available source"], 500);
    }
    if (!$source->isProjectSafe($project)) {
      return new JsonResponse(['message' => "$project->machineName is not safe to add because its security coverage has been revoked"], 500);
    }

However, also of note is that $project->id is displayed in that error message, and looks pretty messed up:

Cannot download drupalorg_jsonapi/drupal-starrating-starrating from any available source

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I'm not convinced this is a major enough usability concern for MVP. We ARE actually providing a default set of filters, so there IS a search being executed, and I believe this would be expected behavior.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

@ressa - first of all, love the stubs in the Handbook, that's exactly what I had in mind.

I totally agree that you want to refer to base instructions in the meta, so that if we ever want to update them we just have to update them in one place.

What is the specific language/wording you want me to look at? I am having trouble digesting what specifically you need feedback on! Thanks for your efforts, this will be amazing. :)

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

@lostcarpark - for the scope of this issue, I'd like the chips just for categories, and in a follow-up I think we could change to "4 selected" or something like that. I still think seeing the chips for categories is useful because then you don't need to open up the select and scroll in order to see which four they are, though.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Like many things that are "star shots" - this is OUT OF THIS WORLD! I'm so overjoyed. I'll celebrate with some fireworks. :)

πŸŽ†

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

chrisfromredfin β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

chrisfromredfin β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

OK, looking at this and trying to give a summary of what needs to happen in this issue to move it along.

  1. Remove the non-category chips altogether. Only categories should show as chips.
  2. Improve the click target so that the entire thing is clickable, and that clicking the box OR the label behaves as the label currently does.

There are some other follow-ups I will file that I think are related to this, but can happen later, in an attempt to keep this issue as tightly scoped as is possible.

  1. Add an "apply" and "clear" button to the bottom of the category selector (we can discuss if this is necessary)
  2. Add "onBlur" type of effect so that when we click off the category dropdown it hides
  3. Improve keyboard accessibility for the dropdown
  4. Change the OTHER dropdown selectors to something more intuitive, like toggle buttons instead of selects/dropdowns (does Claro have a toggle UI pattern?)
  5. Eliminate "bounciness" when category is selected (happens if you're scrolled down and it scrolls back up when the cards disappear as the result of a new search; might make more sense to dim/disable, or keep the containing div at the height it is while new ones load)
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Actually @lostcarpark, I think the answer is to get rid of the chips for anything except for categories. The state of the filter is shown in the dropdown, so I don't think they're needed.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

OK, great. I think that was a consensus needed. This will not be in MVP, but we can also look at revisiting it if it causes confusion after a larger-scale release.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Totally agree. I think we originally assumed everything would have the same TYPES of filters, but not necessarily the same values inside (for example, recipes may have categories, development statues, maintenance status, etc - but they wouldn't have the SAME categories, even if they did).

So I think we have to do this.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Sometimes little things are such big victories. Thanks, everyone!

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Thanks for bearing with us while we floundered on decision-making, everyone! This is a step in a good direction.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

There's been a lot of discussion about backend internals and they further support the idea that this should be a modal. I'm no longer undecided. Anyone who wants to work on the code, please do!

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

This is good incremental improvement, more great stuff to come in the follow-up.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I've been discussing with Adam and I was always a little bit leery of caching the checks, even though I agree that the 90%+ use case nothing would change regardless of what cache lifetime we chose.

I think the right "1.0" approach to this issue is to simply stop calling these checks as frequently. Right now, it seems we are calling the status checks for both the main PB route *and the project detail page route* - and that second one is likely unnecessary and was probably only an accidental byproduct anyway. I believe the intent was to run the status checks only when the "main" UI initialized.

The reality is there that something could change in your setup between browsing and trying an install.

So I'm (a) scoping this issue to simply invoking the status checks less often, but then (b) opening a follow-on / child issue where we will try to come up with an *even better* solution, which may do something like background the process by exposing it as an endpoint that the frontend can invoke. We could even potentially cache the results still, behind that endpoint.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Merged / re-did.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

OK so as @lostcarpark points out in Slack, there are three scenarios to test for:

  1. Core Drupal without Navigation module
  2. Enabled (currently Experimental) Navigation module
  3. Enabled Admin Toolbar module

We are behaving "correctly" for Drupal without Navigation, and WITH Navigation (cases 1 & 2).

We are a bit different for the "Admin Toolbar" module. In fact, with only Admin Toolbar enabled, there are still no sub-children under the "Extend" menu. The module admin_toolbar_tools is in fact what registers those paths on the menu, by providing its own menu links yaml, which registers some of those core routes: https://git.drupalcode.org/project/admin_toolbar/-/blob/3.x/admin_toolba...

So, I think there are two possible approaches here - use a menu alter to add our menu link *only if admin_toolbar_tools is enabled* -OR- punt this upstream to admin_toolbar.

My hunch is that once we're in core, admin_toolbar will probably support adding the menu link to the Browse route in admin_toolbar_extra_tools the same as it does for, say, the current uninstall route.

One possibility to mitigate why this is an issue for us, is to perhaps enable Navigation in our DrupalPod Try It Now - or NOT enable admin_toolbar_extra?

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Re-hashed this in the meeting today @kwiseman. I think the screenshot provided in this Slack thread is the new preferred way. Would you be able to do it this way?

https://drupal.slack.com/archives/C01UHB4QG12/p1719411859068539

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Let's change the order the filters appear in, also -

  1. Categories
  2. Security Advisory Coverage
  3. Maintenance Status
  4. Development Status
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Let's change the order the filters appear in, also -

  1. Categories
  2. Security Advisory Coverage
  3. Maintenance Status
  4. Development Status
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Oh man just being able to have Xdebug on is such a win. ;)

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

There is likely nothing to do for AWS since that's a "VPS" type solution - where you will have the total control you need to configure permission the way they're needed for Project Browser, etc. So I don't think we need to test this one - what we WILL need is to make sure that Project Browser's documentation does document what is needed to set up PB with install functionality on a host where you do have control; but I'll open that as a follow-up.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

This is good work so far, but ideally the output of this is a page in the Handbook related specifically to how to get PB to be fully functional within Dreamhost. Leaving back to NW until that is done.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Re-opening. The point of these is primarily to test it WITH the experimental UI on. That is what will require a lot of work and figuring out if permissions are allowed, etc.

We need someone to be able to turn the module on, turn on Package Manager, and then check the "Allow install" checkbox, and then go to Project Browser and click "Add and Install" with a module, and make the hosting work if possible with that solution.

That's the real work here.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I think trying to get away from Drupalisms like "entities" might make it better. I propose:

"Provides a way to edit related content on the same form where the relationship itself is managed."

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Yes, the proposed resolution I like (from #4).

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

This is only providing an API - so is just a Developer Tools.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine
πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Integrations - since there's an external library it's gluing on; and,

Administration Tools - since it's empowering the site builder to use this library with a no-code tool

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Integrations for sure as top choice. I think it would be worth adding as Administration Tools as well, since it's primarily aimed at the Site Builder for enhancing the UX.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

This belongs in Integrations, since it's gluing an external service onto Drupal.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

With manual testing, prior to applying this MR - all messages show (two status messages and the error). When applied, only the error shows (now correctly as a warning, though). What happened to the other messages?

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Have just played with it real quick and have the following small things I'd love to get cleaned up - but HOLY GOD this is so nice! Thanks!

1. I wish the page wouldn't "jump" when I clicked a filter... I think this is a result of the cards disappearing. If it's an easy fix, great. If not, we can file a follow-up.

2. I think we should get rid of the chips for non-categories, and only show chips for categories. The state of the select shows you what you've chosen there, and I think that's adequate. For categories, I would love it if it would show " selected" if it wasn't all of them, so it would show "3 selected" if you've picked some, ex.g.

3. I would love it if you didn't have to close the dropdown by using the dropdown, but something more traditional like just clicking off-focus as another option would make it a bit more usable (or even clicking escape?! 😬).

This mostly jives with rkoller's feedback, but hopefully is a bit more decisive. My mentality with merging is "is this an incremental improvement, a step forward?" If so, we can go with it then file follow-ups for things as-of-yet unanswered. If we can get at least 2 and 3 in, then it's a substantial incremental improvement, and we'll merge it.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Marking as fixed as this issue's answer has been referenced in the simplytestme issue queue / upstream.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Coming back to reference https://www.drupal.org/project/project_browser/issues/3420698#comment-15... πŸ› PHP memory requirements to install in simplytest.me seem excessive? Fixed - in that I tested and could get it down as far as 192 if that mattered. Looks like we've already discussed 256 also. I am going to close the related issue in Project Browser since that was to get this answer. :)

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

I love that we have the eyeballs to get in there and identify this cleanup stuff.

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Easy win for a11y!

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

Thanks for the breath!

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

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

πŸ‡ΊπŸ‡ΈUnited States chrisfromredfin Portland, Maine

FYI rather than rebuild the entire fixture there's a separate command which will update you with the difference between the code-based fixture and the latest data from d.o.

Project Browser itself is an example of a project you can use the detail page for - because it uses the body template, has images appropriately, has a logo, etc.

Production build 0.69.0 2024