- Issue created by @utkarsh_33
- Merge request !690#3502666: Replace install/select button with a Drupal's action button → (Merged) created by utkarsh_33
- 🇮🇳India utkarsh_33
This is totally a POC for now.Will continue to work on this so keeping it assigned to me.
- 🇺🇸United States phenaproxima Massachusetts
So you are not blocked, my plan is for the backend to send the links as part of the encoded Project objects, like so:
{ // ...other properties of the project... "tasks": [ { "text": "Something", "url": "https://my.site/some/path" }, { // ...another link... } ] }
Pretty simple stuff! But you should be able to rely on that. I'll update here if anything changes.
- 🇺🇸United States phenaproxima Massachusetts
I think this is a great start. Let's block it on ✨ Add instuctions for configuring installed modules Active , which will actually deliver a Configure link for modules, and give us something to really integrate with.
- 🇮🇳India utkarsh_33
So here is the latest update on the current status of the issue:-
- When the module is not installed then we want the normal select button as it was existing before.For that we switched to the Select/Install version of the button as you can see in the SS.
- When the module is installed then we want the button to display the follow-up actions for that(for now it's just a help page's url) which might be expanded in future.The SS of how that looks is attached.
Remaining tasks are:-
- Obviously get the tests fixed.
- Get an initial review of the approach, for that i would love to get a feedback from @phenaproxima or @chrisfromredfin about the approach.I would highly encourage if you can take a pull of the MR and have a look how things are working.
- How should we take care of the Installed status on the cards.For now i have removed the Project status indicator and replaced it with the button(which should be disabled if the module is already installed according to me).If anyone can provide a feedback on that then it would be easier for me to navigate.
- Lastly there was an issue in tests mainly
testDropButtonActions
test.The problem is that even after installing the module the follow-up actions are not getting into the list of dropdown somehow.The tests failing won't give much idea as they are a rough estimate of how things should work once everything is working as expected.Currently the selector might be the issue which i can change if i get to know why the list if follow-up actions is not getting appended.
I know thats a lot of update in a single comment but i how this clarifies what is the current state of the MR and what all is required to be done/needs help with😅.
- When the module is not installed then we want the normal select button as it was existing before.For that we switched to the Select/Install version of the button as you can see in the SS.
- 🇮🇳India utkarsh_33
I tried hard to fix the last failing test but it still fails.Somehow the tasks are coming empty in tests but not locally🥲.
- First commit to issue fork.
- 🇺🇸United States phenaproxima Massachusetts
This seems like a great start and is straightforward. I think we might want to add some test coverage for the keyboard functionality, though. There might be preexisting test coverage in core you could use?
- 🇮🇳India utkarsh_33
Left a few comments explaining some weird stuff in the nightwatch tests.Also the tests seems to fail on waiting for the search results.For now i tried adding a very big random weight so that it passes but it fails on CI consistently but it frequently passes on my local, which makes it a bit off.
- 🇮🇳India utkarsh_33
All the tests are now passing.I think there is still atleast one thing that remains is that we need to disable the dropbutton if there is no follow-up action in some cases.I will take care of that as well.In the mean time I'm marking it NR so that it can undergo a round of review or manual testing.
- 🇺🇸United States phenaproxima Massachusetts
I think this is great. There are some things that could use tightening up and there might be a little missing test coverage, but this broadly seems like it covers all our bases. Amazing work.
- 🇮🇳India utkarsh_33
Addressed all the feedbacks and also added more tests.Marking it NR again.
- 🇮🇳India utkarsh_33
I think we can now remove the tag as the issue summary has the complete description about the issue.
- 🇺🇸United States phenaproxima Massachusetts
I gave this a manual test, and found a few problems that are probably release-blocking, but need not block this commit. These need follow-ups.
First, if you install some modules (that you don't already have), once the installation completes, the badge is rerendered as a dropdown that looks utterly broken:
If you refresh the page, it looks correct. But clicking "Installed" just knocks you back to the top of page without explanation; it feels like a bug.
I tried using the actual links in the dropbutton and they all worked as expected.
- 🇮🇳India utkarsh_33
I have fixed the issue related to
But clicking "Installed" just knocks you back to the top of page without explanation; it feels like a bug
And also related to what you mentioned related to
First, if you install some modules (that you don't already have), once the installation completes, the badge is rerendered as a dropdown that looks utterly broken:
I'm unable to reproduce this.It loads correctly on my local.
I suspect this could be cache issue.When you checkout the branch clear the cache before installing the any module, then it might not happen.I'll ask other community members to test this so that we can get a confirmation if the problem really exists.Marking it NR again as things are working correctly atleast on my end.Let's wait for someone to verify what's happening.
- 🇮🇳India utkarsh_33
Re #24 I tested the issue and i figured out that the issue is not related to the changes that we made in this MR.The issue already exists on 2.0.x.
Steps to test the issue is try to console log the project object in the action button component and see the tasks object once the project is installed(without refreshing ) and then after refreshing.You will see that the url is not correctly passed. - 🇺🇸United States phenaproxima Massachusetts
Gave this another manual test. What I found is that installing a module that's already present (a core module, in this case) does immediately create the correct drop button actions. Clicking "Installed" no longer skips me around the page, so that's good! The actions themselves work as expected -- I'm not sure I understand what's being described in #24, it behaves as I would expect it to. But if it's a pre-existing problem in 2.0.x, let's file a follow-up to deal with that.
The only major commit-blocking problem I still see is that...I can't close the dropbutton once I open it. Clicking the toggle doesn't close it. Clicking outside the drop button doesn't close it. We need to fix these and cover it in the test. Additionally, I am able to open more than one drop button at a time, since they aren't closing reliably. We also need to test that only one can be open at a time.
- 🇺🇸United States phenaproxima Massachusetts
I added some additional test coverage here, and I'm happy with the code here, I think. This is ready for final review and manual testing!
- 🇺🇸United States tim.plunkett Philadelphia
tim.plunkett → made their first commit to this issue’s fork.
- 🇺🇸United States phenaproxima Massachusetts
No follow-up is needed here; everything I found in #21 is fixed.
- 🇺🇸United States tim.plunkett Philadelphia
Re-reviewed, re-tested, all looks good. Awesome work @utkarsh_33!
- 🇺🇸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 phenaproxima Massachusetts
Thanks for the review! Back to @utkarsh_33 to implement the desired changes. Those don't sound too tricky.
- 🇮🇳India utkarsh_33
I have addressed all the feedbacks in #33, but there is on modification that i have done is where the installed icon is placed in comparisons to the dropbutton(if it exists).
I'll attach the SS for both the cases:-1) With follow-up
2) Without Follow-up
This is what i personally think looks good instead of placing them as grid(one over the other).If someone is strictly against this design then I'm happy to change to what’s better in terms of design.
Leaving it to @chrisfromredfin and @phenaproxima for making the decision on designs. - 🇺🇸United States phenaproxima Massachusetts
Regarding #35: I tested both ways and I think I prefer the "Installed" badge being on top, and the drop button being below it. I think this makes more sense semantically, and it also means that very long-titled actions in the drop button won't be disruptive to the design.
I'm trying hard to make sure this lands today, so I think I'm gonna restore RTBC and then review it intensively with @chrisfromredfin.
- 🇺🇸United States phenaproxima Massachusetts
Let's see if I can assign credit with my new issue-maintainer powers...
-
chrisfromredfin →
committed ba9d24f4 on 2.0.x authored by
utkarsh_33 →
Issue #3502666 by utkarsh_33, phenaproxima, tim.plunkett, narendrar,...
-
chrisfromredfin →
committed ba9d24f4 on 2.0.x authored by
utkarsh_33 →
- 🇺🇸United States chrisfromredfin Portland, Maine
cool, and getting cooler!
I may follow a design FU but it shouldn't block this.
Automatically closed - issue fixed for 2 weeks with no activity.