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

Merge Requests

More

Recent comments

🇺🇸United States chrisfromredfin Portland, Maine

chrisfromredfin changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to hidden.

🇺🇸United States chrisfromredfin Portland, Maine

chrisfromredfin changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to active.

🇺🇸United States chrisfromredfin Portland, Maine

chrisfromredfin changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to hidden.

🇺🇸United States chrisfromredfin Portland, Maine

chrisfromredfin changed the visibility of the branch 3317653-rolled-for-10dot4 to hidden.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

Confirmed, already working in Gin 4.0.6 light and dark mode... !

🇺🇸United States chrisfromredfin Portland, Maine

@sarahlorenzen - is there a patch file or an open Merge Request for the work (hopefully an MR)?

🇺🇸United States chrisfromredfin Portland, Maine

Just confirming that Package Manager is not a strict _requirement_ for Project Browser; PB functions without it (though not in as wonderful a way). But it is a soft dependency.

🇺🇸United States chrisfromredfin Portland, Maine

OK, NOW it's Fixed.

🇺🇸United States chrisfromredfin Portland, Maine

Thanks! I will open a follow-up to break out the menu toggle button, since this issue was scoped with two intents.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

I've wanted this for so long; this is such a superior experience in such a subtle way.

🇺🇸United States chrisfromredfin Portland, Maine

NOTE:

  • I'm using a generic "GTM-xxxxx" where x is a digit pattern when installing.
  • My PB settings allow multiple installs (max_selections NULL)
🇺🇸United States chrisfromredfin Portland, Maine

Initial manual testing here seems to imply this is working well. Small pieces of feedback:

(1) the dropbutton with only one action in it looks janky. Not sure if we leave the carat, or shrink the space, or what. But something.

(2) having an issue with 'Drupal2.$dialog is not defined' or something similar. I have seen this in other MRs so I don't think it's related to this issue. But I've seen it enough that it should have a FU.

🇺🇸United States chrisfromredfin Portland, Maine

Release Notes: "BooleanFilter is now a boolean filter."

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

no notes

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

Tim ended up fixing this in a separate issue

🇺🇸United States chrisfromredfin Portland, Maine

I SWORE I already provided feedback on this, but it's not here - might have been one of those ones that I started in an open tab, then closed the tab.

There's a few more major things here, and it might be that this branches out into an issue that needs to be worked on before this one:

1. As Adam says, we need to basically allow a "description" more generically. That's the first piece. FilterBase should support a description field (which accepts markup?).

2. If a description is set, then we should set a ? icon, I think, next to the label (not next to the select). I would use core's questionmark-disc icon (although I'm not sure if we can always count on that being there, but might make sense to try anyway - even if it's not guaranteed).

3. As we're doing now, I think it's OK to then open that description markup in a popup/modal.

4. I'm not sure where we're actually getting the names/descriptions from, but I think we'd want to actually have this queried from Drupal.org; not sure if that means it needs its own endpoint? I'm happy to do that part later, and rely on something like the test fixture (for now), but I don't love that as a long-term solution. (Also, happy to cache that for like an hour or even longer if needed, to help reduce requests to d.o)

🇺🇸United States chrisfromredfin Portland, Maine

back in green, i hit the spleen, it's been to long I'm glad to be seen... 🎵

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

Manually tested in Claro, Gin (Light & Dark). Works great, even fixes an oddity on 2.0.x with Claro. Thank you!

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

tests are passing, PHPStan issue present in 2.0.x-HEAD

🇺🇸United States chrisfromredfin Portland, Maine

PHPStan is from HEAD, so not an issue from this MR.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

I wonder if @lostcarpark could turn this part of the testing into a Nightwatch test? I'm not sure if that would actually help or not. Maybe its scrollIntoView works better, or it handles clicking things that aren't visible better...?

🇺🇸United States chrisfromredfin Portland, Maine

confirmed fixed with manual testing.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

Thanks for the work! Good to see you in the queue, penyaskito!

🇺🇸United States chrisfromredfin Portland, Maine

All my suggestions are being implemented relative to the pager ticket in UI enhancement Active so no need to worry about it here.

🇺🇸United States chrisfromredfin Portland, Maine

Downgrading to stable-blocking because we want this done before we say "we're ready for prime time!" but - it won't have API implications. I think originally was beta when we were considering experimental-in-core as beta.

🇺🇸United States chrisfromredfin Portland, Maine

Postponing, pending results of UX research that is being spearheaded around the tabs & navigation. We need to know the IA before we can continue; although we're not discounting merging core + contrib into one plugin, but we already have a non-API-breaking pattern for that.

🇺🇸United States chrisfromredfin Portland, Maine

No API considerations, moving to stable. HOWEVER, leaving on the SHOULD HAVEs for -beta1.

🇺🇸United States chrisfromredfin Portland, Maine

With no API considerations for this (other than the fact that these keys exist on the Project object, so apply to all sources), pushing back to stable.

🇺🇸United States chrisfromredfin Portland, Maine

After discussing APIs with Adam on Zoom, I think this can safely move to stable blocking. I do consider it a shortcoming that we can't be tolerant, but after looking at all the APIs and where they _might_ change as a result of implementing this, we don't think there's any breakage--if anything, just possibly an addition, if at all.

So I'm re-triaging to stable blocking.

🇺🇸United States chrisfromredfin Portland, Maine

saving to get re-draw ¯\_(ツ)_/¯

🇺🇸United States chrisfromredfin Portland, Maine

Leslie and I just discussed and we think the current experience is acceptable, especially with regard to our target audience (who likely will not have the install functionality disabled anyway), so we'll mark this as Closed (WAD).

If there's an intense clamoring from the community in the future, we'll re-open or re-address.

It's not worth the brain damage of writing all the code and tests for it.

🇺🇸United States chrisfromredfin Portland, Maine

Are there any rules around changing any "JavaScript APIs" for beta for Drupal? This probably got marked beta as a "boy it would be nice" so I'm ok downing it. I'm just gunshy over "API changes" - if we only care about the PHP API, then I'm comfortable because I'm sure most if not all of this solution is on the Svelte side.

🇺🇸United States chrisfromredfin Portland, Maine

love that this is the level of detail we're able to pay attention to now - a testament to our stability!

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

better than status quo. nothing we should really prescribe about when toolbar is active. Using Navigation (as CMS does) will make it a non-issue ;)

🇺🇸United States chrisfromredfin Portland, Maine

confirmed with manual testing that the new additions fix the issues with token when there's a dependency installed. turns out there's a pattern in core where we check dependencies, and then everything else goes to a validator, so we need to replicate that here.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

cool, and getting cooler!

I may follow a design FU but it shouldn't block this.

🇺🇸United States chrisfromredfin Portland, Maine

I'm OK with no test since (a) there's already coverage for this kind of, and (b) I'm SURE Darren will let us know if we regress it ;)

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

manual testing confirms what we already know. need to consider replacing usage of state in tests with key-value, but that's for another day

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

I love where this is headed, but I have two UI concerns.

(1) the "Installed" first line of the dropbutton is non-functional. I think that breaks a pattern we have in Drupal where the first link of a dropbutton is a primary action. I think it's worthwhile to move the "Installed ✔️" to a div ABOVE the dropbutton so that each thing of the drop button is clickable.

(2) This I think solves my other problem, which is that right now on Recipes, they don't have any follow-up tasks, so there's just an "Installed" dropbutton with a carat that you can click and unclick and it flips and unflips, but functionally does nothing. In this way, things without tasks simply don't have buttons and only show the installed.

Quick mockup I fiddled with the Inspector:

..bear in mind, I would love it if "Installed" stayed where it is today, and the dropbutton showed up underneath that, even if it means cards get taller for now where needed.

And one small code nit - I would love it if we called it Dropbutton.svelte instead of DropDownButton.svelte, just to keep in line with Drupal language. We are really trying to mimic/re-invent a common Drupal pattern here in Svelte, so let's intimate that with the language.

🇺🇸United States chrisfromredfin Portland, Maine

RandomData is gone, this was merged with that. I've tested a bunch with all the plugins (even the test plugin - which turns out has some wonks that I might file some novice(?) issues for), but this is way cleaner and easier to understand and better DX.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

thanks for all the eyeballs here, y'all!

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

great spot, lavanyatalwar - thanks for the issue!

🇺🇸United States chrisfromredfin Portland, Maine

Yes, this seems to fix it - I could not reproduce any flickering with this patch.

🇺🇸United States chrisfromredfin Portland, Maine

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

🇺🇸United States chrisfromredfin Portland, Maine

A few comments:

  1. This introduces quite a bit of flickering in the UI that I think is a UI regression/bug.
  2. When activating multiple projects in one fell swoop, it seems to only mark the first one as "Installed." However, the front-end seems to be reacting to the correct statuses from the backend, since the `/activate?_wrapper_format=drupal_ajax` being returned is showing "active" for the first result, but only "present" for the remainder in the list. Note that if you mix-and-match; that is, if you pick two that are already in the filesystem and two that need to be required in, then it activates the local ones first (in the same way, only first one is reflected correctly in the UI), then it activates the required ones (and again shows same behavior, only the first one of that "batch" is reflected correctly in the UI). And again, this is because of what's coming back from the AJAX system. I believe the UI is properly doing "what it's told," but it's being told wrong.
🇺🇸United States chrisfromredfin Portland, Maine

Yes, I would say starting over based on 2.0.x is good.

It's still desirable to be able to click the image or the link. Again, the fundamental issue is being able to wrap those two elements in a single anchor tag in a semantic way, because having two things that click to the same target not good for accessibility.

If there's a way to meet both goals, we go for it. If there's not, then we can abandon and people will just have to click on the title. :)

🇺🇸United States chrisfromredfin Portland, Maine

this is way cool. two follow-ups adam is creating that are UI-related bugs when there's an error, but not directly caused by this code and those will be small, so prefer them in FUs.

🇺🇸United States chrisfromredfin Portland, Maine

j/k this didn't happen after a reinstall. I think my local was just a little messed up.

🇺🇸United States chrisfromredfin Portland, Maine

having an issue installing the RDF module when max_selections == 1

failed request to https://pb11.ddev.site/admin/modules/project_browser/install-require/vJGZ7DQb3s7smsKB7g8ugr3MADpHwq9C: {"message":"StageException: Failed to run process: \u003Cem class=\u0022placeholder\u0022\u003EThe command \u0026quot;\u0026#039;\/var\/www\/html\/vendor\/bin\/composer\u0026#039; \u0026#039;--working-dir=\/tmp\/.package_managerf67b6eaa-503f-4bec-b2fa-795a3972b530\/vJGZ7DQb3s7smsKB7g8ugr3MADpHwq9C\u0026#039; \u0026#039;update\u0026#039; \u0026#039;--with-all-dependencies\u0026#039; \u0026#039;--optimize-autoloader\u0026#039; \u0026#039;drupal\/rdf\u0026#039;\u0026quot; failed.\n\nExit Code: 2(Misuse of shell builtins)\n\nWorking directory: \/var\/www\/html\/web\n\nOutput:\n================\n\n\nError Output:\n================\nLoading composer repositories with package information\nUpdating dependencies\nYour requirements could not be resolved to an installable set of packages.\n\n  Problem 1\n    - Root composer.json requires drupal\/rdf ^2.1 -\u0026gt; satisfiable by drupal\/rdf[2.1.0, 2.1.1, 2.x-dev].\n    - drupal\/rdf[2.1.0, ..., 2.x-dev] require drupal\/core ^9.4 || ^10.0 -\u0026gt; found drupal\/core[9.4.0-alpha1, ..., 9.5.x-dev, 10.0.0-alpha1, ..., 10.5.x-dev] but these were not loaded, likely because it conflicts with another require.\n\n\u003C\/em\u003E","phase":"require"} Response {type: 'basic', url: 'https://pb11.ddev.site/admin/modules/project_brows…/install-require/vJGZ7DQb3s7smsKB7g8ugr3MADpHwq9C', redirected: false, status: 500, ok: false, …}
popup.js:56 Uncaught (in promise) TypeError: Drupal$1.dialog is not a function
    at openPopup (popup.js:56:29)
    at handleError (InstallListProcessor.js:87:3)
    at doRequests (InstallListProcessor.js:181:15)
    at async processInstallList (InstallListProcessor.js:223:5)
    at async HTMLButtonElement.onClick (ActionButton.svelte:47:42)
🇺🇸United States chrisfromredfin Portland, Maine

cool, this needed a follow-up but that's been filed.

🐛 Remove filter definitions from ProjectBrowserSourceBase Active

🇺🇸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

greeeeeen is my new favorite color

🇺🇸United States chrisfromredfin Portland, Maine

More than anything on this one, the comments are the real MVP.

🇺🇸United States chrisfromredfin Portland, Maine

noice. what would've been noicer is using what was already there, but 🤷

🇺🇸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

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

🇺🇸United States chrisfromredfin Portland, Maine

This is good with contains for me as it's supported by Mink.

Production build 0.71.5 2024