-
Notifications
You must be signed in to change notification settings - Fork 433
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: Staking banner #4396
feat: Staking banner #4396
Conversation
Branch preview✅ Deploy successful! Storybook: |
📦 Next.js Bundle Analysis for safe-wallet-webThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
Page | Size (compressed) |
---|---|
global |
1009.76 KB (🟡 +30 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script>
tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
Fifteen Pages Changed Size
The following pages changed size from the code in this PR compared to its base branch:
Page | Size (compressed) | First Load |
---|---|---|
/apps |
48.42 KB (🟡 +1 B) |
1.03 MB |
/apps/custom |
40.06 KB (🟡 +1 B) |
1.03 MB |
/balances |
38.17 KB (🔴 +8.73 KB) |
1.02 MB |
/balances/nfts |
19.2 KB (-1 B) |
1 MB |
/home |
65.47 KB (🟡 +8.13 KB) |
1.05 MB |
/settings |
450 B (🟢 -1 B) |
1010.2 KB |
/settings/cookies |
7.68 KB (🟡 +1 B) |
1017.45 KB |
/settings/data |
7.58 KB (🟡 +3 B) |
1017.34 KB |
/settings/environment-variables |
9.17 KB (🟢 -3 B) |
1018.93 KB |
/settings/modules |
9.84 KB (🟡 +3 B) |
1019.61 KB |
/settings/notifications |
27.05 KB (🟡 +2 B) |
1.01 MB |
/settings/setup |
36 KB (🟡 +2 B) |
1.02 MB |
/transactions |
73.69 KB (-1 B) |
1.06 MB |
/transactions/history |
73.66 KB (-1 B) |
1.06 MB |
/transactions/queue |
31.31 KB (🟡 +2 B) |
1.02 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link
is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script>
tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Coverage report
Test suite run success1573 tests passing in 208 suites. Report generated by 🧪jest coverage report action from 3506b64 |
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.
Looks good!
I think we need to remove the swap banner and place staking in its place.
const router = useRouter() | ||
const { balances } = useBalances() | ||
|
||
const nativeTokenBalance = balances.items.find((balance) => balance.tokenInfo.type === TokenType.NATIVE_TOKEN) |
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 guess it doesn't matter here as the balances won't be that huge of an array, but not having this memoized or not in a separate hook that is executed just once means that this line will be constantly recomputed for whatever reason the widget rerenders.
|
||
const [widgetHidden = false, setWidgetHidden] = useLocalStorage<boolean>(LOCAL_STORAGE_KEY_HIDE_WIDGET) | ||
|
||
const isStakingFeatureEnabled = useIsStakingFeatureEnabled() |
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.
@schmanu maybe instead of looking whether staking is enabled we could have a separate feature flag for only the banner? This way we can turn it off one day without doing a new release of the app. Or too much? What do you think? We don't do this for the swaps banner now and we'll have to remove it. I think the staking banner is supposed to take the Swap place.
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 would say its a product question. Not sure if we want to keep the other widgets 🤷
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.
Andrei confirmed that the swap banner should be replaced by the new one. I'm on it.
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.
In the designs there is also a small banner in the Assets view which is missing here.
right: 0; | ||
} | ||
|
||
.gradiantBackground { |
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.
Nit:
.gradiantBackground { | |
.gradientBackground { |
border-radius: 50%; | ||
} | ||
|
||
.gradiantBackgroundDarkMode { |
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.
Nit:
.gradiantBackgroundDarkMode { | |
.gradientBackgroundDarkMode { |
|
||
const [widgetHidden = false, setWidgetHidden] = useLocalStorage<boolean>(LOCAL_STORAGE_KEY_HIDE_WIDGET) | ||
|
||
const isStakingFeatureEnabled = useIsStakingFeatureEnabled() |
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 would say its a product question. Not sure if we want to keep the other widgets 🤷
I removed the swap banner and added a smaller version of the staking banner to be rendered on the assets page. |
@tmjssz can you rebase. This should fix the failing test. |
Show a banner for staking on the dashboard if the user holds >= 32 ETH.
Remove the swap banner in favor of the the staking banner
…ons (dafault + large) * large version is rendered on the dashboard * default version is rendered on the assets page
1e40c2c
to
3506b64
Compare
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 Safe Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
The feature is checked , but there is a ticket that should be implemented before the Staking banner release - https://www.notion.so/safe-global/Staking-banner-notes-mobile-view-filtering-don-t-show-again-1298180fe57380768fb1e90d283fc54a?pvs=4 |
Show banners for staking on the dashboard and the assets page if the user holds >= 32 ETH.
What it solves
Resolves SW-191
How this PR fixes it
This pull request adds a new Staking banner as a replacement of the Swap banner. The changes include implementing the StakingBanner component, integrating it into the dashboard and assets pages, and updating the styles and hooks accordingly.
Staking Banner Implementation:
src/components/dashboard/StakingBanner/index.tsx
: Added a newStakingBanner
component with support for both large and small views. This component includes logic to check for sufficient funds (>= 32 ETH), dark mode, and the possibility to hide it.src/components/dashboard/StakingBanner/styles.module.css
: Added CSS styles for theStakingBanner
component, including responsive design and dark mode support.src/features/stake/hooks/useIsStakingBannerEnabled.ts
: Created a new hookuseIsStakingBannerEnabled
to determine if the staking banner feature is enabled.Integration:
src/components/dashboard/index.tsx
: Integrated theStakingBanner
component into the dashboard, replacing the Swap Widget. [1] [2]src/pages/balances/index.tsx
: Added theStakingBanner
component to the assets page. [1] [2]Removal of Swap Widget:
src/features/swap/components/SwapWidget/index.tsx
: Removed theSwapWidget
component and its associated logic.Additional Updates:
src/services/analytics/events/overview.ts
: Added new analytics events for the staking banner interactions.src/utils/chains.ts
: AddedSTAKING_BANNER
to theFEATURES
enum.How to test it
Load the app for an account that has >= 32 ETH and visit the dashboard and assets pages where the banner should appear at the top. Alternatively, update the
MIN_NATIVE_TOKEN_BALANCE
constant in theStakingBanner
component.Screenshots
Dashboard
Assets
Checklist