Replace install/select button with a Drupal's action button

Created on 28 January 2025, 2 months ago

Problem/Motivation

Steps to reproduce

Proposed resolution

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇮🇳India utkarsh_33

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue 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.

  • Pipeline finished with Failed
    2 months ago
    Total: 413s
    #407907
  • Pipeline finished with Failed
    2 months ago
    Total: 653s
    #419707
  • 🇺🇸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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 461s
    #420587
  • Pipeline finished with Failed
    about 2 months ago
    #420588
  • Pipeline finished with Failed
    about 2 months ago
    Total: 539s
    #421668
  • Pipeline finished with Failed
    about 2 months ago
    Total: 490s
    #421734
  • Pipeline finished with Failed
    about 2 months ago
    Total: 702s
    #421747
  • Pipeline finished with Success
    about 2 months ago
    Total: 766s
    #421754
  • 🇮🇳India utkarsh_33

    Finally the tests are passing🥳🥳🥳.

  • Pipeline finished with Success
    about 2 months ago
    Total: 479s
    #421815
  • 🇮🇳India utkarsh_33

    This is how the action button looks like🥳.

  • Pipeline finished with Success
    about 2 months ago
    Total: 449s
    #421840
  • 🇮🇳India utkarsh_33

    This is ready for a review now.

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

  • Pipeline finished with Success
    about 2 months ago
    Total: 482s
    #422852
  • 🇺🇸United States phenaproxima Massachusetts

    Blocker is in!

  • Pipeline finished with Failed
    about 2 months ago
    Total: 584s
    #423969
  • Pipeline finished with Failed
    about 2 months ago
    Total: 636s
    #423989
  • Pipeline finished with Failed
    about 2 months ago
    Total: 610s
    #424011
  • 🇮🇳India utkarsh_33

    So here is the latest update on the current status of the issue:-

    1. 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.
    2. 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:-

    1. Obviously get the tests fixed.
    2. 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.
    3. 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.
    4. 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😅.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 336s
    #426272
  • Pipeline finished with Failed
    about 2 months ago
    Total: 499s
    #426275
  • Pipeline finished with Failed
    about 2 months ago
    Total: 355s
    #426302
  • 🇮🇳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🥲.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 3702s
    #426411
  • Pipeline finished with Failed
    about 2 months ago
    Total: 713s
    #427068
  • Pipeline finished with Failed
    about 2 months ago
    Total: 688s
    #427076
  • Pipeline finished with Failed
    about 2 months ago
    Total: 568s
    #427079
  • Pipeline finished with Failed
    about 2 months ago
    Total: 601s
    #427090
  • Pipeline finished with Failed
    about 2 months ago
    Total: 725s
    #427100
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 826s
    #427107
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 1036s
    #427114
  • Pipeline finished with Failed
    about 2 months ago
    Total: 1895s
    #427126
  • Pipeline finished with Failed
    about 2 months ago
    Total: 396s
    #427146
  • Pipeline finished with Failed
    about 2 months ago
    Total: 357s
    #427160
  • Pipeline finished with Failed
    about 2 months ago
    Total: 491s
    #427164
  • First commit to issue fork.
  • Pipeline finished with Failed
    about 2 months ago
    Total: 504s
    #427387
  • Pipeline finished with Success
    about 2 months ago
    Total: 488s
    #427417
  • 🇺🇸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?

  • Pipeline finished with Canceled
    about 2 months ago
    Total: 228s
    #428277
  • Pipeline finished with Failed
    about 2 months ago
    Total: 457s
    #428279
  • Pipeline finished with Failed
    about 2 months ago
    Total: 438s
    #428287
  • Pipeline finished with Failed
    about 2 months ago
    Total: 537s
    #428299
  • Pipeline finished with Failed
    about 2 months ago
    Total: 429s
    #428309
  • Pipeline finished with Failed
    about 2 months ago
    Total: 381s
    #428313
  • Pipeline finished with Failed
    about 2 months ago
    Total: 448s
    #428321
  • Pipeline finished with Failed
    about 2 months ago
    Total: 2734s
    #428338
  • 🇮🇳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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 3984s
    #429429
  • Pipeline finished with Failed
    about 2 months ago
    Total: 404s
    #429487
  • Pipeline finished with Failed
    about 2 months ago
    Total: 5839s
    #429543
  • Pipeline finished with Failed
    about 2 months ago
    Total: 6105s
    #429648
  • Pipeline finished with Failed
    about 2 months ago
    Total: 529s
    #430328
  • Pipeline finished with Failed
    about 2 months ago
    Total: 378s
    #430455
  • Pipeline finished with Failed
    about 2 months ago
    Total: 492s
    #430465
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 627s
    #430471
  • Pipeline finished with Failed
    about 2 months ago
    Total: 877s
    #430478
  • Pipeline finished with Success
    about 2 months ago
    Total: 701s
    #430484
  • 🇮🇳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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 398s
    #430577
  • Pipeline finished with Failed
    about 2 months ago
    Total: 425s
    #430583
  • Pipeline finished with Failed
    about 2 months ago
    Total: 786s
    #430588
  • Pipeline finished with Success
    about 2 months ago
    Total: 407s
    #430599
  • 🇺🇸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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 995s
    #430783
  • Pipeline finished with Failed
    about 2 months ago
    Total: 484s
    #432382
  • Pipeline finished with Success
    about 2 months ago
    Total: 496s
    #432386
  • Pipeline finished with Failed
    about 2 months ago
    Total: 367s
    #432438
  • Pipeline finished with Failed
    about 2 months ago
    Total: 463s
    #432464
  • Pipeline finished with Failed
    about 2 months ago
    Total: 607s
    #432483
  • Pipeline finished with Failed
    about 2 months ago
    Total: 364s
    #432494
  • Pipeline finished with Failed
    about 2 months ago
    Total: 508s
    #432498
  • Pipeline finished with Success
    about 2 months ago
    Total: 452s
    #432593
  • 🇮🇳India utkarsh_33

    Addressed all the feedbacks and also added more tests.Marking it NR again.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 542s
    #433130
  • Pipeline finished with Failed
    about 2 months ago
    Total: 613s
    #433143
  • Pipeline finished with Failed
    about 2 months ago
    Total: 327s
    #433151
  • Pipeline finished with Success
    about 2 months ago
    Total: 366s
    #433152
  • 🇮🇳India utkarsh_33

    Addressed feedbacks🥳.

  • Pipeline finished with Success
    about 2 months ago
    Total: 499s
    #433170
  • 🇮🇳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.

  • 🇺🇸United States phenaproxima Massachusetts
  • Pipeline finished with Success
    about 1 month ago
    Total: 605s
    #435350
  • 🇮🇳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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 1193s
    #435384
  • Pipeline finished with Success
    about 1 month ago
    Total: 780s
    #435395
  • 🇮🇳India narendraR Jaipur, India

    Re #23, Tested manually and it is working on my end. One problem which I see is that Uninstall should take you to current page and not /admin/modules/uninstall after a module is installed and you try to uninstall it without refreshing the page.

  • 🇮🇳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
  • Pipeline finished with Failed
    about 1 month ago
    Total: 344s
    #435756
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 224s
    #435762
  • Pipeline finished with Success
    about 1 month ago
    Total: 371s
    #435767
  • Pipeline finished with Canceled
    about 1 month ago
    Total: 262s
    #435814
  • 🇺🇸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!

  • Pipeline finished with Success
    about 1 month ago
    Total: 426s
    #435821
  • 🇺🇸United States phenaproxima Massachusetts
  • Pipeline finished with Success
    about 1 month ago
    Total: 363s
    #435958
  • 🇺🇸United States tim.plunkett Philadelphia

    tim.plunkett made their first commit to this issue’s fork.

  • Pipeline finished with Success
    about 1 month ago
    Total: 479s
    #435966
  • Pipeline finished with Success
    about 1 month ago
    Total: 366s
    #435971
  • 🇺🇸United States phenaproxima Massachusetts

    No follow-up is needed here; everything I found in #21 is fixed.

  • Pipeline finished with Success
    about 1 month ago
    Total: 538s
    #436011
  • 🇺🇸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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 493s
    #436274
  • Pipeline finished with Failed
    about 1 month ago
    Total: 285s
    #436353
  • Pipeline finished with Failed
    about 1 month ago
    Total: 485s
    #436356
  • Pipeline finished with Success
    about 1 month ago
    Total: 459s
    #436359
  • Pipeline finished with Success
    about 1 month ago
    Total: 361s
    #436368
  • 🇮🇳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.

  • 🇮🇳India utkarsh_33

    Marking it NR.

  • Pipeline finished with Success
    about 1 month ago
    Total: 470s
    #436772
  • 🇺🇸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...

  • Pipeline finished with Canceled
    about 1 month ago
    Total: 319s
    #436782
  • Pipeline finished with Success
    about 1 month ago
    Total: 878s
    #436788
  • Pipeline finished with Skipped
    about 1 month ago
    #436951
  • 🇺🇸United States chrisfromredfin Portland, Maine

    cool, and getting cooler!

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

  • 🇺🇸United States phenaproxima Massachusetts
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024