-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: display no extension banner on streaming payments page #4120
base: feat/streaming-payments-ui
Are you sure you want to change the base?
feat: display no extension banner on streaming payments page #4120
Conversation
…ng-permissions Feat: Cancel streams using permissions
…ing stream claim amounts
…ent is the sender
fix: refresh available funds
…ents-widgets feat: add streaming payments widgets
…ay-extension-error Fix/16227916 streaming display extension error
feat: add teams filtration for streaming payments page
fix: extensions order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isStreamingPaymentsInstalled = | ||
extensionData && isInstalledExtensionData(extensionData); | ||
|
||
const inStreamingPaymentsDisabled = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny spelling mistake: isStreamingPaymentsDisabled
(You might already have it, but just in case, I use this spell checker VS Code extension which helps me catch issues like this all the time! https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adam-strzelec nice start on this issue 🙌
I left a few refactoring comment, if you could have a look over them, I'd totally appreciate it 👍
<a href={`${COLONY_EXTENSIONS_ROUTE}/${Extension.StreamingPayments}`}> | ||
{formatMessage(MSG.link)} | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using the <a>
tag, we're going to trigger a whole-page reload
Screen.Recording.2025-01-17.at.12.32.30.mov
Let's refactor this and use the Link
component.
Also for the extension url we could use const { extensionUrl } = useExtensionItem(Extension.StreamingPayments);
const isStreamingPaymentsInstalled = | ||
extensionData && isInstalledExtensionData(extensionData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this piece of code can be easily refactored to use
const { isExtensionInstalled } = useExtensionItem(
Extension.StreamingPayments,
);
Given it performs already all these checks to verify if the extension was or not installed.
extensionData && isInstalledExtensionData(extensionData); | ||
|
||
const inStreamingPaymentsDisabled = | ||
!!extensionData && !isStreamingPaymentsInstalled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to check if extensionData
exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6196803
to
af9aebd
Compare
Description
Display banner info about no extension installed on streaming payments page if extension is not installed
Testing
Diffs
New stuff ✨
Changes 🏗
Resolves #4096