-
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
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
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.
@adam-strzelec Thank you for the PR, the banner looks good. Although, it does not fully meet the issue without the "Create new stream" button.
Also, the banner should show when the extension is in a deprecated state as well. At the moment, it only shows if the extension is not installed.
Not Installed
Deprecated
@adam-strzelec this branch needs a rebase before I can drop another review 🙌 |
29c1854
to
7aeb3f8
Compare
@arrenv Is there a design somewhere that shows what the banner should look like when the extension is deprecated? |
Sure, the banner and design accounts for both cases. The message says "Streaming payments extension is currently not enabled." So, when it is not installed or deprecated, then that means it is not enabled. |
Description
Display banner info about no extension installed on streaming payments page if extension is not installed
Testing
Diffs
New stuff ✨
Changes 🏗
Resolves #4096