Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Feature/1730 notif banner miss adjusts #1756

Merged
merged 10 commits into from
Nov 10, 2021

Conversation

maria-vslvn
Copy link
Contributor

Summary

Fixes #1730

Notification banner is expected to disappear after it was closed and a page was reloaded.

To Test

To reproduce:

  1. Connect a wallet in the Mainnet with the app
  2. Open profile page and copy a referral link
  3. Paste the link into the browser
  4. Close the info banner
  5. Refresh the page

AR: banner appears again

ER: banner should not appear when it is closed for the current account
Need to show banner again when switch to another account

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @maria-vslvn , I cannot to close the banner now: the page quickly refreshes, then close button stops working.
https://watch.screencastify.com/v/H9BVgKFmzeP1QNNA3STz

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments regarding the code.

Regarding the behaviour, if I dismiss one banner regarding reason x for account a, when I change to account b, reason x will not pop a banner again.

I think the banner dismiss status should be kept per account, but maybe I missed where this was decided.

Not strongly against it, just raising the point. Does anyone agree?

@elena-zh
Copy link

elena-zh commented Nov 3, 2021

I think the banner dismiss status should be kept per account, but maybe I missed where this was decided.
Not strongly against it, just raising the point. Does anyone agree?

@alfetopito , it should work as you described: statuses states should be kept per account.
It was described in the main task (#1730)

@ramirotw ramirotw added the Protofire Handled by Protofire development team label Nov 5, 2021
@ramirotw ramirotw changed the base branch from release/1.4.0 to develop November 9, 2021 13:50
@maria-vslvn maria-vslvn changed the base branch from develop to release/1.4.0 November 9, 2021 14:00
@maria-vslvn
Copy link
Contributor Author

@elena-zh @alfetopito @ramirotw please check the updates

@elena-zh
Copy link

elena-zh commented Nov 9, 2021

Current behavior does not look like an expected: I have created a separate issue for this #1806

}

export const initialState: AffiliateState = {
appDataHash: APP_DATA_HASH,
isNotificationClosed: IS_NOTIFICATION_CLOSED,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the direction we want to go!

We just need 2 more dimensions: account and reason/type of notification

With the current setup, if dismissed once, the pop-up will never be displayed again for other accounts or reasons

@ramirotw ramirotw changed the base branch from release/1.4.0 to hotfix/1.4.2 November 10, 2021 09:10
Copy link
Contributor

@ramirotw ramirotw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remaning issues are tackled in #1806

@ramirotw ramirotw merged commit 9cd5f8f into hotfix/1.4.2 Nov 10, 2021
@ramirotw ramirotw deleted the feature/1730-notif-banner-miss-adjusts branch November 10, 2021 09:17
@ramirotw ramirotw mentioned this pull request Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Protofire Handled by Protofire development team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants