Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/16227916 streaming display extension error #4071

Conversation

adam-strzelec
Copy link
Contributor

Description

Display and handle streaming payments extension error when the extension is not installed

Testing

Create Streaming payment action without extension installed.

Diffs

Screenshot 2025-01-10 at 15 31 28

Resolves #4052

@adam-strzelec adam-strzelec requested a review from a team as a code owner January 10, 2025 14:37
@CLAassistant
Copy link

CLAassistant commented Jan 10, 2025

CLA assistant check
All committers have signed the CLA.

@adam-strzelec adam-strzelec changed the base branch from master to feat/streaming-payments-ui January 10, 2025 14:37
@adam-strzelec adam-strzelec self-assigned this Jan 10, 2025
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this one @adam-strzelec 💯

The notification regarding the Streaming payments extension is correctly showing up if the extension is not installed

Screen.Recording.2025-01-13.at.10.12.54.mov

And no longer showing up after it is installed

Screen.Recording.2025-01-13.at.10.23.12.mov

And the Reputation extension banner still shows up the the Create agreement
Screenshot 2025-01-13 at 10 23 53

I left a small refactoring comment though and would appreciate to hear your thoughts 🙌

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work @adam-strzelec 🙌

I would propose to refactor a bit the way we handle the link to the extension page.

What do you think if we'll extract this helper function

export const getNeededExtension = (action: Action) => {
  switch (action) {
    case Action.CreateDecision:
      return Extension.VotingReputation;
    case Action.StreamingPayment:
      return Extension.StreamingPayments;
    default:
      return '';
  }
};

from src/components/v5/common/ActionSidebar/partials/hooks.ts
and then use it here

  const extensionId = getNeededExtension(selectedAction);
  const extensionLink = `${COLONY_EXTENSIONS_ROUTE}/${extensionId}`;

....

              <button
                  type="button"
                  onClick={() => {
                    navigate(extensionLink);
                    toggleActionSidebarOff();
                  }}
                >
                  {formatText(MSG.viewExtension)}
                </button>

@adam-strzelec adam-strzelec requested a review from mmioana January 13, 2025 10:47
Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All working as expected!

When uninstalled:
Screenshot 2025-01-13 at 11 46 32

When installed:
Screenshot 2025-01-13 at 11 47 05

Create agreement is still working as intended:
Screenshot 2025-01-13 at 11 50 39

Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your fast changes @adam-strzelec 🥇

I've already tested them, so I'll approve your PR 💯

Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adam-strzelec Nice one, working as expected, showing the message when the extension is not enabled.

image

@adam-strzelec adam-strzelec merged commit 52f79d8 into feat/streaming-payments-ui Jan 15, 2025
3 of 5 checks passed
@adam-strzelec adam-strzelec deleted the fix/16227916-streaming-display-extension-error branch January 15, 2025 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming payment action can be triggered without the extension installed
5 participants