- 🇺🇸United States tim.plunkett Philadelphia
This has a legit merge conflict after 🐛 Selected categories disappear on switching tabs Active went in, not just on the compiled svelte.
- 🇺🇸United States chrisfromredfin Portland, Maine
Woohoo! Testing this caused me to notice separate issues with the Core plugin but they are present on 2.0.x without this MR. And this MR seems to fix the issue the way I caused it. And RTBC with two other manual tests, so here it goes!
-
chrisfromredfin →
committed dbb3b281 on 2.0.x authored by
narendrar →
Issue #3481084 by narendrar, chrisfromredfin, rkoller, utkarsh_33:...
-
chrisfromredfin →
committed dbb3b281 on 2.0.x authored by
narendrar →
- First commit to issue fork.
- 🇺🇸United States chrisfromredfin Portland, Maine
Wonderful! A bigger victory than the code suggests. :)
Meanwhile, we have to find a way to not bikeshed language decisions. But, this works! When in doubt, make it more minimal.
-
chrisfromredfin →
committed 52cc76c2 on 2.0.x authored by
utkarsh_33 →
Issue #3318791 by utkarsh_33, fjgarlin, divya.sejekan, rkoller,...
-
chrisfromredfin →
committed 52cc76c2 on 2.0.x authored by
utkarsh_33 →
Automatically closed - issue fixed for 2 weeks with no activity.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇺🇸United States teknorah Mokena, IL, US
What about instead of a drop down sort by option on the grid view, having a sortable updated field (by clicking on the column title) on the list view? This way, the special use case would be covered, but maybe less confusing in the sort by options?
- 🇺🇸United States leslieg
While we were doing discovery. this was added as a sort due to people asking - how do I find modules that were added recently so I can decide if there is something newer than what I am currently using. Not sure how many foks would actualy use it. Maybe we start with "sort by relevance" only and then re-address this option if folks start asking for it. Interested in @chrisfromredfin thoughts
- 🇺🇸United States drumm NY, US
Is this sort order actually needed? What is the use case for it? Will many people use it?
- @utkarsh_33 opened merge request.
- First commit to issue fork.
- 🇺🇸United States chrisfromredfin Portland, Maine
Oof, yes, overthink. I thought placeholder text was a "recommended" pattern but actually if it's not adding clarity, yes, just boot it and leave it "Search" as the label. That solves it across every plugin. Thanks for the light, @phenaproxima - sometimes we need to be reminded we're too deep in the weeds, but we're too deep in the weeds to see out.
- 🇺🇸United States phenaproxima Massachusetts
I wonder if we are overthinking this.
I think most people know what to do when presented with a search query box. It's the commonest of patterns. You enter some text there, it finds stuff that matches. I don't think they need to know that it searches module names, descriptions, whatever. That's the job of the app to figure out.
So IMHO there need not be any placeholder text at all. I think this might be a case where trying to be too helpful will actually be less helpful.
- 🇩🇪Germany rkoller Nürnberg, Germany
Sorry, I've taken a look at the latest state of the MR, the placeholder text in the current form is not helpful at all. For contrib modules the placeholder text tells the following: as a person new to drupal, it tells me nothing at all. i dont know what
field group
mean and recaptcha also doesnt ring a bell, i only know there are captchas on the web. as a person experienced with drupal, it tells me ah i am able to search for module names and nothing else but module names in this field. and searching for eitherrecaptcha
orfield group
inrecipes
provides me zero results, same for thecore modules
. - 🇳🇱Netherlands idebr
#1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS → has been fixed in Drupal core, so this issue is no longer postponed.
I tested the MR and the issue reported here is fixed, also the code changes looks good to me so marking it RTBC.
- First commit to issue fork.
Automatically closed - issue fixed for 2 weeks with no activity.
- 🇩🇪Germany rkoller Nürnberg, Germany
oh i havent realized that those empty selected filters with the undefined chips could happen on and off! i always ran right away into it as soon as i've switched to the core modules source type. never tried switching tabs repeatedly, so never fully grasped that the hiding can switch on and off. at first i thought this issue wouldnt be reproducible and about something else than i've raised in the slack on thread. but understood now the steps to reproduce and also am able to confirm that the MR603 fixes the issue. thank you! only tested the MR functionally/manually, leave it at needs review that someone else can also review the code.
- 🇩🇪Germany rkoller Nürnberg, Germany
thanks @narendraR, i've just tested your latest changes and i can confirm as soon as the last checkbox in a multiselect is unchecked the results count is set back to the number of all available modules for the the source type in question.
- @narendrar opened merge request.
- Issue created by @utkarsh_33
I have added back the wrench icon in this issue. 📌 Decide on iconography for maintenance status Needs work .If we don't want to continue with legend then we can either get the remaining work done in the same issue or we can wait for that to merge and then carry on the remaining work on this issue.Marking this needs review for now so that we have next steps clear on this, as this is a beta blocker.
This now has some work align to what @chrisfromredfin mentioned in #31 📌 Decide on iconography for maintenance status Needs work .Marking it needs review.
Now the icons are of the same size as you can see in the attached screenshots.
- 🇩🇪Germany rkoller Nürnberg, Germany
the pattern @lostcarpark discovered can be pushed even further. after you've clicked for example the token module leading to that grey background. you then click another module for the first time, after you close that modal again and try to open/click that other module also a second time the background. You can proceed that pattern with any other module card. every time the background gets even darker on every second try for a module card. looks like transparent background get added onto each other..
and i've also tried when the greyish background shows up for the first time to try to press esc in case that is some sort of "weird" modal instance and that grey background is some sort of rudiment of a modal. but pressing esc had no effect at all and can be ruled out.
and in regards of the modal width as mentioned in the corresponding discussion on slack i would go with a max width between 80 and 90rem instead of 90vw. i have no strong opinion for or against anything inbetween 80 to 90rem, any of those values would work. my problem with 90vw, on widescreen monitors the line length becomes way too long.
- 🇮🇪Ireland lostcarpark
I've also found a fairly significant bug. If you open the detail modal for a project, then try to open it again, it fails to reopen, and for some reason, also changes the background colour of the Project Browser page to grey. You can still open other project detail pages, but once you've opened a detail page once, you can't open it again without a page refresh.
- 🇮🇪Ireland lostcarpark
I did some work on the ESLINT issues.
It didn't like assigning
DetailModal
to a variable, and then not using it again, but it also didn't like creating it withnew
and not assigning a variable.I think it's probably better assigning an unused variable, because it looks a bit more normal. I've added
// eslint-disable-next-line no-unused-vars
to keep eslint quiet.However, I wonder is that the best approach?
- 🇺🇸United States leslieg
Thanks @chrisfromredfin. That's an interesting question. I'll bring this up for discussion at next Tuesday's Sitebuilder Subcommittee meeting.
My personal opinion is to use creation date of the project on drupal.org. Since the default filters are "Covered by Security Policy" and "Maintained", then the list of newest projects should eliminate projects that aren't ready for prime time yet. (Advanced filters would still allow this option if someone wanted to see all new projects.)
I think using "last_updated" as the sort would always promote the largest modules as they are constantly updating their codebases. Maybe that isn't true all the time, however I don't think it's what people have asked for when using this sort.
Will update here after we meet on Tuesday.
- 🇺🇸United States chrisfromredfin Portland, Maine
Still relevant in that we want to support it as a sort-order, but we need to get that data from the backend. I'm not sure if the recency of updated that people are looking for is the recency of updating the actual project page, though. So we don't want to use the node updated date of the module_project node.
I think what we actually want is something like "date of last commit to the repo in any way" or "recent release." However, I'm also not sure if people want this from the perspective of wanting to find _NEW_ modules only, in which case it makes more sense to sort by the module_project node's _created_ date.
I am going to punt this to the Site Builders Subcommittee for their next meeting and leave it postponed for now.
I know @mandclu has opinions on this, also.
- 🇺🇸United States chrisfromredfin Portland, Maine
I'm not 100% that there should be a special case of what "carries over" - maybe nothing should. however, I think we should advance this issue, and if UX people bring it up later, we'll file it then (I don't even think we should file a FU now, even). I will test and review.
- 🇺🇸United States phenaproxima Massachusetts
@narendrar kindly got on Zoom with me to explain the nature of the problem and what he did. Thanks to his clear explanation, I see what was going on -- the filters applied on any given tab were being applied globally (that is, to all tabs), which of course would filter out all results on other tabs. The tests weren't catching this because they were asserting the wrong thing.
Naren demonstrated that the adjusted test fails on 2.0.x HEAD, so all good there!
The changes in the code now make sense to me; we're keeping all filters localized to particular tabs, except for the keyword search, which is globally applied. The tests appear to cover that this works.
A little manual testing couldn't hurt but I'm too busy with other things for that, and I'm satisfied that this does what's advertised and fixes the bug. Therefore, RTBC!
- 🇪🇸Spain fjgarlin
I could help with this and build some tooling to make it easier in the future but I won’t be able to get to it in a few days/weeks.
That should be ok since the change is kind of tied to the improvements needed in the endpoint (which I’ll be doing 😅).
But if anyone wants to give it a go I’m happy to review.
This still needs some CSS fixing as there is a difference in heights between the 2 icons.
I'll ask @tim.plunkett to change the target branch to 2.0.x as i don't have the permission of doing that and then maybe work on this to get it more refined.
Assigning it to Tim for above mentioned task.I have created a new MR which implements what is suggested in the proposed resolution, thought we might need some CSS to style the button as per Drupal standards but still marking this as NR to get initial feedbacks on the work.
utkarsh_33 → changed the visibility of the branch 3315859-communicated-the-statusstate to hidden.
- @utkarsh_33 opened merge request.
I have updated the placeholder and label according to the discussion on the issue.But i still think @narendrar made a valid point in #20 📌 Improve and clarify the microcopy for the search field placeholder Needs review that we should consider the recipes as well as the search component is generic.I'll mark this NR for another round of review.