-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: better storybook stories for the notification pages #27861
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [cf66689]
Page Load Metrics (1909 ± 109 ms)
Bundle size diffs
|
Quality Gate passedIssues Measures |
Builds ready [2322c20]
Page Load Metrics (1795 ± 141 ms)
Bundle size diffs
|
Builds ready [36288c3]
Page Load Metrics (1948 ± 98 ms)
Bundle size diffs
|
Builds ready [7f9b8c6]
Page Load Metrics (1990 ± 97 ms)
Bundle size diffs
|
Builds ready [4096501]
Page Load Metrics (1875 ± 42 ms)
Bundle size diffs
|
Builds ready [d44e45b]
Page Load Metrics (1868 ± 64 ms)
Bundle size diffs
|
Builds ready [14cacbc]
Page Load Metrics (1879 ± 64 ms)
Bundle size diffs
|
ui/pages/notifications/notification-components/feature-announcement/feature-announcement.tsx
Show resolved
Hide resolved
Builds ready [312be96]
Page Load Metrics (1740 ± 51 ms)
Bundle size diffs
|
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.
Probably you don't need an additional env variable to check if the process is running under Storybook
headers: process.env.STORYBOOK_ENV | ||
? undefined | ||
: { | ||
'Infura-Source': 'metamask/metamask', |
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.
Storybook registers certain global variables that you can check for during runtime. For instance, Storybook adds a window.STORYBOOK_ADDONS_CHANNEL property when it runs.
You can use this to conditionally detect Storybook like so:
const isStorybook = typeof window !== 'undefined' && !!window.__STORYBOOK_ADDONS_CHANNEL__;
This way, you won’t need the STORYBOOK_ENV variable; instead, you’re checking for a runtime indication that you’re in Storybook.
c8ec8d1
Builds ready [843ef49]
Page Load Metrics (1987 ± 57 ms)
Bundle size diffs
|
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.
a couple of code optimisation comments
); | ||
}; | ||
|
||
export const EthSent = Template.bind({}); |
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.
what about reducing redundancy and something like:
// Dynamic export generation
const notificationStories = Object.keys(processedNotifications).reduce(
(acc, key) => {
acc[key] = Template.bind({});
acc[key].args = { notification: processedNotifications[key] };
return acc;
},
{}
);
export const {
ethSent: EthSent,
ethReceived: EthReceived,
erc20Sent: ERC20Sent,
erc20Received: ERC20Received,
erc721Sent: ERC721Sent,
erc721Received: ERC721Received,
erc1155Sent: ERC1155Sent,
erc1155Received: ERC1155Received,
lidoReadyToBeWithdrawn: LidoReadyToBeWithdrawn,
lidoStakeCompleted: LidoStakeCompleted,
lidoWithdrawalCompleted: LidoWithdrawalCompleted,
lidoWithdrawalRequested: LidoWithdrawalRequested,
metaMaskSwapsCompleted: MetaMaskSwapsCompleted,
rocketPoolStakeCompleted: RocketPoolStakeCompleted,
rocketPoolUnStakeCompleted: RocketPoolUnStakeCompleted,
featureAnnouncement: FeatureAnnouncement,
} = notificationStories;
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.
Nice cleanup, lets either add this now or in the next subsequent PR.
For context, the number of notification types we will support is going to grow substantially over time. So being able to create storybooks for each one easily (without much effort) will be important.
- Web 3 Notifications:
aave_v3_health_factor
;ens_expiration
;lido_staking_rewards
;notional_loan_expiration
;rocketpool_staking_rewards
;spark_fi_health_factor
- Price Alerts: Native Token Alerts, ERC Alerts (token/nft alerts)
- And more (security, etc)
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.
Thanks, Andrea! Actually, I was thinking of optimizing these stories in a new PR I’m working on because we’re introducing new stories (as @Prithpal-Sooriya mentioned). But you’re right; better to do it right away!
}, | ||
]); | ||
|
||
const { |
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.
similar approach here, doing something like:
const mockNotifications = {
ethSent: NotificationServicesController.Mocks.createMockNotificationEthSent,
ethReceived:
NotificationServicesController.Mocks.createMockNotificationEthReceived,
erc20Sent: NotificationServicesController.Mocks.createMockNotificationERC20Sent,
erc20Received:
NotificationServicesController.Mocks.createMockNotificationERC20Received,
erc721Sent:
NotificationServicesController.Mocks.createMockNotificationERC721Sent,
erc721Received:
NotificationServicesController.Mocks.createMockNotificationERC721Received,
erc1155Sent:
NotificationServicesController.Mocks.createMockNotificationERC1155Sent,
erc1155Received:
NotificationServicesController.Mocks.createMockNotificationERC1155Received,
metaMaskSwapsCompleted:
NotificationServicesController.Mocks.createMockNotificationMetaMaskSwapsCompleted,
rocketPoolStakeCompleted:
NotificationServicesController.Mocks.createMockNotificationRocketPoolStakeCompleted,
rocketPoolUnStakeCompleted:
NotificationServicesController.Mocks.createMockNotificationRocketPoolUnStakeCompleted,
lidoStakeCompleted:
NotificationServicesController.Mocks.createMockNotificationLidoStakeCompleted,
lidoWithdrawalRequested:
NotificationServicesController.Mocks.createMockNotificationLidoWithdrawalRequested,
lidoReadyToBeWithdrawn:
NotificationServicesController.Mocks.createMockNotificationLidoReadyToBeWithdrawn,
lidoWithdrawalCompleted:
NotificationServicesController.Mocks.createMockNotificationLidoWithdrawalCompleted,
featureAnnouncement:
NotificationServicesController.Mocks.createMockFeatureAnnouncementRaw,
};
const notifications: Notification[] = Object.values(mockNotifications).map(
(createMock) => processNotification(createMock()),
);
Builds ready [15c45d9]
Page Load Metrics (2311 ± 116 ms)
Bundle size diffs
|
1162ecf
Builds ready [1162ecf]
Page Load Metrics (1978 ± 54 ms)
Bundle size diffs
|
Description
This pull request enhances the Storybook stories for the notification pages. The improvements aim to provide better clarity and usability for developers working with notification components. The changes include more detailed and organized stories, making it easier to understand and test different notification states and components.
### Important
ui/pages/notification-details/notification-details.stories.tsx
) need to use an RPC endpoint to fetch certain dataINFURA_STORYBOOK_PROJECT_ID
env (.metamaskrc.dist
)Infura-Source
header is not used in the utility (ui/helpers/utils/notification.util.ts
)Related issues
N/A
Manual testing steps
Screenshots/Recordings
Before
N/A
After
Screen.Recording.2024-10-24.at.17.23.49.mov
Pre-merge author checklist
Pre-merge reviewer checklist