I realized, while reviewing this again, that one of the major reasons the code feels awkward is because it's taking place in ProjectGrid.svelte, which is totally disconnected from the loading code.
Is there any chance we can move this stuff (mostly) into ProjectBrowser.svelte? It understands when loading is happening, and that, to me, feels like a much more natural place to call scrollIntoView().
Thanks for helping me analyze this, @utkarsh_33. 🙏
I'm not entirely certain here. The current number of flags still feels somewhat...imprecise to me, and we need to keep the Svelte code as clean as we can because it is prone to messiness. :)
I'd like to see what happens if we start removing flags.
✨ Allow recipes to be re-"installed" (re-applied) inside PB Active is in!
phenaproxima → created an issue.
phenaproxima → created an issue.
phenaproxima → created an issue.
Two should-haves remaining...
Opened ✨ Improve the layout of the filters Active to improve the layout of the filters.
phenaproxima → created an issue.
Getting the layout right is proving surprisingly tricky. @kunal.sachdev and I paired on it, then I tried to hack on it myself, and neither of us was able to get anywhere.
What really needs to happen here is a stronger layout - the boolean filters should be grouped together, as a list, and to the right of the categories. But that's beyond the scope of this issue.
So we agreed to land this as-is, even with the layout being sub-optimal, since we both feel this is a better IA than a row of select lists. I will open a follow-up to implement a better layout for the filters, which can be a must-have issue for beta2.
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?).
Yup, let's add a new string|\Stringable|null $description = NULL parameter to FilterBase::__construct(). There's no reason it couldn't take markup.
This would be an API addition, not an API break, and therefore this issue is not a beta blocker.
Test written - I think we're good here.
We landed our final must-have!
Confirmed that tests catch the bug; the test-only job fails as expected.
Figured out the problem -- clicking the label causes the problem; clicking the checkbox does not. The tests normally click the checkbox.
Ensuring credit is correct, as @narendra and @utkarsh_33 worked to reproduce this in a test while I was asleep.
Quickly checked in Gin and Claro - the alignment looks much better! Ship it.
Drupal CMS does set it to /home: https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/recipes/drupal_...
If I do a clean install of Drupal CMS 1.x, then run ddev drush cget system.site, I see this:
page:
403: /user/login
404: ''
front: /node/2
The front page is being set as I would expect, but I would get redirected to /user/login if I didn't have permission to access /node/2. But the default front page is published, so even as an anonymous user, it seems to work okay for me.
What am I missing here?
the code is in two repositories
Not to derail this issue, but can you elaborate on this?? There's only one repository for Drupal CMS (this project that we're posting in right now). Which two repositories are you referring to?
I can also confirm that if I pause the test at the beginning with a sleep(30) and click around, my proposed fix appears to prevent the bug.
I did a little bit of investigating and discovered that the reason the tests aren't catching is because...even in 2.0.x, the test behaves correctly!
And yet, if I put a sleep(30) at the top of the test, and then click around manually in Chrome...I am seeing the buggy behavior. In other words, this is a case where the human interaction of clicking on the checkbox causes the bug, but Mink's interaction with the checkbox does not.
That leads me to believe that the problem is something to do with, maybe, the onChange function in MultipleChoiceFilter.svelte. There are some really dodgy DOM traversal/queries happening in showHideFilter to deal with focusing -- those seem very suspicious to me, and it's entirely possible that they're somehow looking at the wrong part of the DOM tree.
I think we need to take a few steps back here and start with a test that unambiguously fails against 2.0.x. We're not going to have any other way to know for sure if we've fixed this bug.
Another question: how is it that the existing test didn't catch this bug?
At the bottom of the test method in 2.0.x, we are confirming that adding an additional category to the second instance produces a different list of results than the first instance. Why didn't this fail in 2.0.x?
Fixed merge conflicts, and suggested an alternate (simpler) approach here that might work.
The bigger problem, though, is that the test passes locally even against 2.0.x...so it's not catching the bug. :( The test needs to fail on 2.0.x without the patch.
kristen pol → credited phenaproxima → .
Yowza! Merged into 1.x. Thanks all!
Gave this a quick manual test and it pretty much works as advertised. It's much more in line with what you'd expect from a modern CMS!
Since I didn't really write any code here, I'm calling it.
I have nothing to complain about here from a code perspective! I think if someone can give it a quick manual test to confirm everything looks as it should, I'm good to merge it.
After dialing back the scope and reverting the CSS changes, we have this:
I don't think this is too bad; in fact, I think it's enough to call this issue fixed, and we can rework the layout in a follow-up.
The only tweak I would suggest is that the checkbox label should not wrap around the checkbox itself; it should always be fully next to it. I'm not sure what the correct CSS for this would be.
The bigger problem is that, once I uncheck the checkbox, I...can't check it again! That's problematic, and most likely a bug in the JavaScript. Reassigning to @kunal.sachdev to figure that out.
At @kunal.sachdev's request, I tried to figure out the grouping stuff here. It really turned into a rabbit hole.
As I see it, the problem is that "groups" has no real meaning to FilterBase, and its purpose is therefore ambiguous. The Svelte code doesn't use it for anything.
Therefore, I think the way forward here is to reduce the scope a bit. Let's not worry about grouping the filters. Let's not worry about stacking them on top of each other. Let's keep things in exactly the same layout they are now, except that the boolean filters become checkboxes. We'll also remove FilterBase::$group, since it's not used and we're not sure how to use it. We can always restore it later, if we need to, as an API addition.
I'll take care of reducing the scope here, and keeping Kunal's changes to BooleanFilter.svelte and the tests. I think we can keep the CSS basically as it is in 2.0.x.
Refreshing! We landed more stuff.
This looks pretty close to perfect, but there are a few things we need to fix. We're missing the dependency in drupal_cms_content_type_base, and the test changes, while fine, don't actually give us anything new -- so we can revert them for now, I think.
I discussed this with @poker10 in Slack.
While I appreciate that we definitely do not want Composer to ever accidentally choose a non-covered version of a project, I feel fairly strongly that surfacing the incoming versions to our users is the correct solution to this problem.
Why? Because, well...an incoming version number is probably utterly meaningless to most of Project Browser's intended audience. Unless a user is willing to research the specifics about an incoming version (and I think we can safely assume they won't)...what do we expect people to do with the information? I can't think of anything.
All this will do, I feel, is add friction to Project Browser, and reinforce the perception that you need a Ph.D. to use Drupal.
But as I said, the underlying motivation here seems legitimate. Composer has no concept of "not covered by security updates", so there is nothing to prevent it from choosing poorly.
What I would suggest instead is, internally, the Installer::require() method should try to find a reasonable version constraint to give to Composer, basing that on inspecting the configuration in composer.json and cross-referencing that with metadata gleaned from the Update Manager module (which is a dependency of Package Manager). That way we can ensure we never allow non-covered versions. Another way to accomplish this would be to add a conflict section to the sandboxed composer.json, which would explicitly disallow versions that the Update Manager module states are not covered by security advisories. And we can let Composer sort things out from there.
I also don't think this is truly critical. It's a major thing, and potentially blocks a stable release of Project Browser (a decision I will leave to the maintainers), but I don't see any particular justification for critical.
None that I can detect. Tests are failing but I don't think it's related. So, merged into 1.x and cherry-picked to 1.0.x. Thanks everyone!
This looks like a good start but the other points in #18 still need to be done. Quoting for convenience:
- Adds the module to admin_ui and content_type_base
- Uses the widget, on the Tags field only, for the content types that we ship with Drupal CMS
- Adjusts tests so that they pass (this will be needed; we are checking which widget is used for each field)
Gave this a quick manual test and confirmed that the selector looks awful in 2.0.x with Gin's dark mode enabled, and much nicer with this MR applied!
The CSS doesn't look problematic to me either. As far as I can tell, we're good to go here.
I dig this change. I think it increases the amount of polish - nice work.
I gave it a quick manual test on a page with one instance of Project Browser, but didn't test it with multiple instances yet.
The code looks reasonably straightforward but feels a little overcomplicated for what it's doing. It also needs more comments, and there appear to be out-of-scope test changes in there (which, to be honest, were probably introduced by me while I was fixing merge conflicts).
This looks pretty good in both light and dark mode under Gin, and it looked pretty good in Claro, but there is one thing that I think should be fixed before commit -- namely, the "Installed" badge and drop button are a little bit off-center. The effect is particularly exacerbated in Gin, for some reason.
It's a minor detail, to be sure, but I think it's in the scope of this issue since we're just trying to ensure that these UI components look good, with proper dimensions and positioning.
Re #16: That sounds useful later, but let's start small. For now, no need for a custom config action or automatic reapply. We could create a simple MR right this minute which brings in Tagify and uses it on the existing Tags fields. Later on we can add the more dynamic stuff.
So let's start with an MR that does the following:
- Adds the module to admin_ui and content_type_base
- Uses the widget, on the Tags field only, for the content types that we ship with Drupal CMS
- Adjusts tests so that they pass (this will be needed; we are checking which widget is used for each field)
Re #15: I think we might want to handle base fields separately, since they can specify whether or not their form display is even configurable to begin with. Truth is, for Drupal CMS's purposes, having Tagify on the Tags field is probably the most important case to cover, so if we need to deal with adding to base fields (like uid later), that's probably fine to do in a follow-up.
Re #11: That sounds quite a bit more complicated than I was envisioning. I need to do a little research about it, but I was envisioning that there would simply be a single boolean flag somewhere (tagify.settings:suggest_as_default or whatever) which, if enabled, makes Tagify the default widget suggested whenever a supported field is created, but otherwise imposes no opinion or requirement to use it. I imagine that'd be relatively easy to do and wouldn't require a detailed settings object with entity type-specific settings.
Thoughts?
I have no strong opinion on what Tagify does here, to be clear -- that is up to you as maintainers. But as far as Drupal CMS is concerned, this would probably cover our use nicely.
Re-titling for clarity, and tagging to make clear that this will be in Drupal CMS 1.1.0, but not 1.0.x, since it is a feature addition.
I'm glad Pam is on board with this. I am too.
I think the module should be added to Admin UI for general availability, and drupal_cms_content_type_base since it would be used in all content types by default. Let's also update the form displays in the individual content type recipes.
IMHO, ensuring that Tagify is used for all new reference fields is something that we can do in a follow-up issue, once ✨ SEO Tools recipe should automatically reapply itself every time a content type is created Active is in and provides an example. Although, to be honest, my preference would be for Tagify itself to support this, maybe via a config flag if possible. It would be good enough for it to simply make itself the default widget for the field type(s) that it supports. Not 100% sure how to do that but I'm pretty sure there is a way.
So why don't we use this issue to add the module to our recipes, and open a second issue (postponed on this one) to use it by default on new fields.
kristen pol → credited phenaproxima → .
I really like what we're doing here. It's so much clearer and it makes the tests much easier to grok.
I think the only sticking point, here, is how we group the filters. We need this to be more generic. I posted a suggestion on how we might go about that.
Also there are now some merge conflicts to be fixed since we removed clickWithWait() and pressWithWait() from the tests.
The content is already translatable. Default Content exports translations and core imports them, and I'm pretty sure we have test coverage of that.
Didn't bother giving this a review because it won't affect existing sites, it's only used within XB, and we are planning to iterate more on it anyway. Blissfully merged into 1.x, thanks all.
The Svelte code is not an API, full stop. There is no way that I'm aware of for any external code to extend or integrate with it. All of the API exists on the PHP side.
So we can change the frontend whenever we want.
phenaproxima → created an issue.
We'll need a test for this (a new method of MultipleInstancesTest should suffice), and it's definitely a beta blocker.
#3 gives me an idea: we could add a new AJAX command that forces all visible instances of Project Browser to reload their data, using the previously known set of query parameters.
Activators which know that dependencies were enabled -- and we could add a trait for activators to easily figure this out -- could return this command, and that would get everything that was installed, to show up as installed.
It's a bit brute force, but I'm not sure there's a cleaner way to do this. I've been puzzling over this one for days and the architecture just don't allow it to be done using RefreshProjectsCommand.
Assigning to myself to see if this can work at all.
Needs tests, but I found an approach that makes this work without major UI behavior changes and extensive refactoring.
Long story short: allow follow-up tasks to take advantage of the use-ajax class.
Wow, this one is turning out to be really effing hard to pull off.
I'm going to back off and try to think of an approach that will actually work.
phenaproxima → changed the visibility of the branch 3489729-allow-recipes-to to hidden.
kristen pol → credited phenaproxima → .
Test cases that need to happen here:
- A recipe with no input should trigger a batch job and redirect back to the page we were on.
- A recipe with input should show a form, and once that form is submitted, it should trigger a batch job and redirect back to the page we were on. If there are validation errors, the form should always refresh until those are resolved.
Merged into 1.x, thanks! In the name of caution, not backporting to 1.0.x, but I can be persuaded if there's a strong case for it in the near term.
Yes, it will work if the default changes. Config actions always do what they say they'll do, regardless of what the config might already be.
Another day, another several issues landed! Re-saving to refresh.
Gave this a quick manual test and confirmed that, although both buttons work in 2.0.x with Gin as the theme, they do indeed look broken. The patch fixes it, and the buttons continue to be functional. As far as I can tell, this is good to go.
The prompt is part of core's installer: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
Drupal CMS suppresses it entirely with a form_alter hook: https://git.drupalcode.org/project/drupal_cms/-/blob/1.x/project_templat...
We suppress it because, as has been pointed out, we always enable Automatic Updates and Project Browser, which have a dependency on Package Manager, which in turns depends on Update Manager.
We could surely adjust it so that we don't expose the checkbox -- which would have no effect anyway -- but we do keep the disclosure, or some re-worded version of it. Not sure that it would suffice, though, since the truth is that there's no real way to opt-out unless you also are willing to remove Automatic Updates along with Project Browser's "install from the UI" functionality.
phenaproxima → changed the visibility of the branch 3511417-remove-projectbrowseruitesttraitclickwithwait to hidden.
phenaproxima → created an issue.
I do like the proposed resolution, although I think we should remove the bit that scrolls you directly to the projects, as @narendrar pointed out in #6. Let's just remove that outright.
The test changes do make me a bit nervous, though -- we are, if I remember correctly, now bypassing all of these waits. I sorta wonder why this causes the tests to pass.
This is frankly why I'd like to remove clickWithWait() and replace it with something better. Not yet sure what that would look like, though.
@cmlara We can't really get rid of Update Manager, it's a hard dependency of Package Manager and therefore Automatic Updates (and Project Browser). That's a mission-critical set of modules.
If you're willing to roll a stable release of https://www.drupal.org/project/site_key_mutator → , could we not just include that with Drupal CMS (maybe as part of the basic privacy recipe)? Would that mitigate the concerns here until core can fix the problem more fully?
Assigning to @pameeela for final review.
Assigning to @pameeela for review.
Fixing the tests is gonna take some doing, since this is a definite logic change and will alter the number of projects that come up, and which projects come up, in any given search.
Adding ✨ Allow the ProjectBrowser render element to control which sort options are shown Active and ✨ [PP-1] Allow the ProjectBrowser render element to customize the available sort options Postponed as should-haves, since they will allow mini-browsers to exercise more control over the UI chrome.
phenaproxima → created an issue.
I'm not clear on why this is tagged as a beta blocker.
Beta is where the PHP API and update path begin, but this has nothing to do with either of those. The frontend code is completely internal and we can change it however we like, whenever we like, so I'm tentatively untagging this as a beta blocker...but certainly a worthwhile feature!
I'd like to advocate for not making this a beta blocker. It shouldn't change our PHP API, and the Svelte code is internal, so although this would be a UX improvement, it could surely be a post-beta feature.
Moving 📌 Consider removing the development status filter Active to the could-have list, based on the findings in #3506565-2: Consider removing the development status filter → .
After some discussion with @chrisfromredfin, @fjgarlin, and @drumm, I think this is probably a bigger issue that should be postponed and rescoped.
@drumm made the point that, on drupal.org, "actively developed" is a "neutral" status. One does not say "this module is actively developed!" -- it simply refers to any module that has not been explicitly marked as minimally maintained, obsolete, insecure, or some other "negative" status.
To me, this suggests that Project Browser's IA could be simplified, such that we remove the development status filter while at the same time changing the "actively maintained" filter to instead -- internally -- be a filter that equates to "not obsolete, and not minimally maintained, and not insecure, etc."
But that does not need to block anything. The frontend is internal, and what filters sources define is also completely up to them.
One more must-have is committed, along with a should-have, and another must-have is ready for review.
Sending this one directly to Tim for review, since this is pure maintainer stuff.
Discussed with @tim.plunkett and we agreed to remove this without an update path. It's already been marked obsolete in a tagged release (alpha10), and this module doesn't actually do anything useful for a real-world site and it never has. Anybody who enabled it from Project Browser 1.0.x is...well, why would they even do that? I have no idea why anybody would want a source that spits out randomly generated crap.
So, this flies with my blessing.
Two more must-haves got in!
Blocker is committed, so assigning this one to @narendrar.