Fix: refactor + don't show push notifications banner if wallet not conencted #2604
Conversation
…n't track banner dismissal on enable all/customize
Branch preview✅ Deploy successful! |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
There was a problem hiding this comment.
I like the approach of extracting some of the logic into local hooks. It would be nice to have tests for them, especially useShowBanner to make sure the change with the connected wallet works without manually testing it but I will approve the PR.
I've checked that the dismiss event only happens when the user closes the banner but not when enabling all or customizing. The other 2 events are still being sent correctly.
katspaugh
changed the title
|
I won't be merging this because we need to release these and other fixes today, so I'll make a more focused PR with just the fixes. |
| <CheckWallet> | ||
| {(isOk) => ( | ||
| <div className={css.buttons}> | ||
| <Button | ||
| variant="contained" | ||
| size="small" | ||
| className={css.button} | ||
| onClick={onEnableAll} | ||
| disabled={!isOk} | ||
| > | ||
| Enable all | ||
| </Button> | ||
|
|
||
| <Link passHref href={{ pathname: AppRoutes.settings.notifications, query }} onClick={onCustomize}> | ||
| <Button variant="outlined" size="small" className={css.button}> | ||
| Customize | ||
| </Button> | ||
| </Link> | ||
| </div> | ||
| )} | ||
| </CheckWallet> |
There was a problem hiding this comment.
As we are no longer showing the banner when no wallet is connected, we don't need to wrap this in CheckWallet.
Also don't track banner dismissal on enable all/customize.
What it solves
The banner was previously shown regarless of whether you're connected which clashed with the onboard popup.