All of this sounds good to me. Only thing that I take a quick note of is that we already have the "dependency popularity" problem with modules... Chaos Tools being one of the top-installed modules... that users almost never care about.
Our consideration for this was to maybe allow projects to opt into a checkbox like "library-only" and then hide those from PB's default filters (allow them to be shown if the user wants that).
Not sure that applies to recipes, as most base recipes ARE "useful unto themselves," so maybe it's different. Anyway, just a little something to consider/think about.
Thank you, libbna! Good progress. A couple things I'm noticing.
(1) The apply/clear bar should be "sticky" at the bottom of the container that opens. That is, it should always be visible and only the list of checkboxes should be scrollable. That can probably also be a little less tall. Maybe something like a height of whatever 4.5 rows is, with maybe a max-height of like 90vh or something?
(2) the "Clear" button should act as a submit button. So, while it clears the checkboxes, you still have to scroll back down and click Apply, which shouldn't actually be necessary. Hitting clear should wipe out all the category selections and cause a projects refresh.
chrisfromredfin → changed the visibility of the branch 3458840-consider-adding-apply to hidden.
chrisfromredfin → changed the visibility of the branch 3515746-change-language-around to hidden.
I think because the fix is in Svelte code, we need a FunctionalJavascriptTest to render the JS.
It seems to me like the answer is to *improve* the config form, then. The recipe config form for AI should prompt not just for an OpenAI key, but for a Provider + Key. No?
chrisfromredfin → created an issue.
This does fix the problem so I'm going to merge it now. But I think if we can rely on native Drupal api's like Drupal.url() we need to do that, so I'll open that as a follow-up.
In sveltejs/constants.js, we use "export const FULL_MODULE_PATH = `${BASE_URL}${drupalSettings.project_browser.module_path}`;" and FULL_MODULE_PATH is then used to get the path to the image.
BASE_URL uses "export const BASE_URL = `${window.location.protocol}//${window.location.host}${drupalSettings.path.baseUrl + drupalSettings.path.pathPrefix}`;"
...does drupalSettings.path.pathPrefix include a language prefix?
Or, does the core moduleHandler do it?
`'module_path' => $this->moduleHandler->getModule('project_browser')->getPath(),`
...I'm pretty sure it's probably drupalSettings.path.pathPrefix, which is meant for the "Drupal is installed in a subfolder" scenario; however, if it's a language prefix, that should NOT be included.
Does anyone know how core handles this kind of scenario, where it needs a path to the module without the language prefix?
This will need a test.
chrisfromredfin → changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to hidden.
chrisfromredfin → changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to active.
chrisfromredfin → changed the visibility of the branch drupal-3317653-3317653-rolled-for-10dot4 to hidden.
chrisfromredfin → changed the visibility of the branch 3317653-rolled-for-10dot4 to hidden.
chrisfromredfin → made their first commit to this issue’s fork.
Confirmed, already working in Gin 4.0.6 light and dark mode... !
@sarahlorenzen - is there a patch file or an open Merge Request for the work (hopefully an MR)?
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.
chrisfromredfin → created an issue.
chrisfromredfin → created an issue.
OK, NOW it's Fixed.
chrisfromredfin → created an issue.
Thanks! I will open a follow-up to break out the menu toggle button, since this issue was scoped with two intents.
chrisfromredfin → made their first commit to this issue’s fork.
I've wanted this for so long; this is such a superior experience in such a subtle way.
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)
Updated IS with steps to reproduce
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.
Release Notes: "BooleanFilter is now a boolean filter."
chrisfromredfin → made their first commit to this issue’s fork.
no notes
chrisfromredfin → made their first commit to this issue’s fork.
Tim ended up fixing this in a separate issue
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)
back in green, i hit the spleen, it's been to long I'm glad to be seen... 🎵
chrisfromredfin → made their first commit to this issue’s fork.
Manually tested in Claro, Gin (Light & Dark). Works great, even fixes an oddity on 2.0.x with Claro. Thank you!
chrisfromredfin → made their first commit to this issue’s fork.
tests are passing, PHPStan issue present in 2.0.x-HEAD
chrisfromredfin → made their first commit to this issue’s fork.
PHPStan is from HEAD, so not an issue from this MR.
chrisfromredfin → created an issue.
chrisfromredfin → created an issue.
chrisfromredfin → made their first commit to this issue’s fork.
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...?
confirmed fixed with manual testing.
chrisfromredfin → made their first commit to this issue’s fork.
Thanks for the work! Good to see you in the queue, penyaskito!
chrisfromredfin → made their first commit to this issue’s fork.
All my suggestions are being implemented relative to the pager ticket in ✨ UI enhancement Active so no need to worry about it here.
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.
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.
No API considerations, moving to stable. HOWEVER, leaving on the SHOULD HAVEs for -beta1.
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.
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.
saving to get re-draw ¯\_(ツ)_/¯
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.
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.
That'll do pig, that'll do.
chrisfromredfin → made their first commit to this issue’s fork.
love that this is the level of detail we're able to pay attention to now - a testament to our stability!
chrisfromredfin → made their first commit to this issue’s fork.
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 ;)
chrisfromredfin → made their first commit to this issue’s fork.
yessireebob
chrisfromredfin → made their first commit to this issue’s fork.
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.
chrisfromredfin → made their first commit to this issue’s fork.
cool, and getting cooler!
I may follow a design FU but it shouldn't block this.
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 ;)
chrisfromredfin → made their first commit to this issue’s fork.
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
chrisfromredfin → made their first commit to this issue’s fork.
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.
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.
chrisfromredfin → made their first commit to this issue’s fork.
thanks for all the eyeballs here, y'all!
chrisfromredfin → made their first commit to this issue’s fork.
great spot, lavanyatalwar - thanks for the issue!
chrisfromredfin → made their first commit to this issue’s fork.
Yes, this seems to fix it - I could not reproduce any flickering with this patch.
chrisfromredfin → made their first commit to this issue’s fork.
A few comments:
- This introduces quite a bit of flickering in the UI that I think is a UI regression/bug.
- 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.
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. :)
feels good to me; I reviewed & so did Tim.
chrisfromredfin → made their first commit to this issue’s fork.
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.
j/k this didn't happen after a reinstall. I think my local was just a little messed up.
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)
chrisfromredfin → made their first commit to this issue’s fork.
cool, this needed a follow-up but that's been filed.
🐛 Remove filter definitions from ProjectBrowserSourceBase Active
chrisfromredfin → made their first commit to this issue’s fork.
it's easy being green
chrisfromredfin → made their first commit to this issue’s fork.
greeeeeen is my new favorite color
More than anything on this one, the comments are the real MVP.
noice. what would've been noicer is using what was already there, but 🤷
chrisfromredfin → made their first commit to this issue’s fork.
chrisfromredfin → created an issue.