Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: move pin extension notification to main screen #902

Merged
merged 3 commits into from
Feb 29, 2024
Merged

feat: move pin extension notification to main screen #902

merged 3 commits into from
Feb 29, 2024

Conversation

greatertomi
Copy link
Contributor

Checklist

  • JIRA - LW-9747
  • Proper tests implemented
  • Screenshots added.

Proposed solution

move the pin extension notification from the final onboarding step to the main screen

Testing

Describe here, how the new implementation can be tested.
Provide link or briefly describe User Acceptance Criteria/Tests that need to be met

Screenshots

Screenshot 2024-02-20 at 13 52 47

@greatertomi greatertomi requested a review from a team as a code owner February 20, 2024 12:57
@github-actions github-actions bot added the browser Changes to the browser application. label Feb 20, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 20, 2024 •
edited
Loading

Allure report

allure-report-publisher generated test report!

smokeTests: ❌ test report for a80bda75

passed failed skipped flaky total result
Total 27 5 0 0 32 ❌

@coveralls
Copy link

coveralls commented Feb 20, 2024 •
edited
Loading

Coverage Status

Changes unknown
when pulling a80bda7 on feat/lw-9747-pin-extension
into ** on main**.

szymonmaslowski
szymonmaslowski approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@szymonmaslowski szymonmaslowski left a comment

Choose a reason for hiding this comment

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

💪

@szymonmaslowski
Copy link
Contributor

I think we should start counting 5 seconds after the user closes the dapp connector popup. Now the pin extension element is barely visible and probably will disappear before the user finishes reading the modal content. It would be good to have a generic mechanism checking if the screen is not covered otherwise there might be a different modal in the future covering this pin extension thing.
image

The second thing I noticed is that this element covers entirely the account dropdown so the user has no possibility to interact with it. Maybe it could have a close button, or an indicator for when it is going to be hidden, similar to our toasts elements (see below).
image

cc: @vhulchenko-iohk

danielmain
danielmain approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@danielmain danielmain left a comment

Choose a reason for hiding this comment

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

I left a question about the constant

apps/browser-extension-wallet/src/views/browser-view/components/Layout/Layout.tsx Show resolved Hide resolved
...sion-wallet/src/views/browser-view/features/wallet-setup/components/PinExtension.module.scss Show resolved Hide resolved
@greatertomi
Copy link
Contributor Author

The second thing I noticed is that this element covers entirely the account dropdown so the user has no possibility to interact with it. Maybe it could have a close button, or an indicator for when it is going to be hidden, similar to our toasts elements (see below).

Is this important since the notification will close in 5s?

@vhulchenko-iohk
Copy link

I think we should start counting 5 seconds after the user closes the dapp connector popup. Now the pin extension element is barely visible and probably will disappear before the user finishes reading the modal content. It would be good to have a generic mechanism checking if the screen is not covered otherwise there might be a different modal in the future covering this pin extension thing. image

The second thing I noticed is that this element covers entirely the account dropdown so the user has no possibility to interact with it. Maybe it could have a close button, or an indicator for when it is going to be hidden, similar to our toasts elements (see below). image

cc: @vhulchenko-iohk

Hey @szymonmaslowski, great points. Chatting with the UI/UX designer about them. If OK and agreed, I will have a separate Jira ticket to add these suggested changes. OK?

cc @greatertomi

@greatertomi greatertomi force-pushed the feat/lw-9747-pin-extension branch from d8f2c94 to 1c94112 Compare February 26, 2024 10:28
szymonmaslowski
szymonmaslowski requested changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@szymonmaslowski szymonmaslowski left a comment

Choose a reason for hiding this comment

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

One more small request 😉

apps/browser-extension-wallet/src/views/browser-view/components/Layout/Layout.tsx Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Feb 26, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

emiride
emiride approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@emiride emiride left a comment

Choose a reason for hiding this comment

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

Works as expected. It is good to go.

@greatertomi greatertomi changed the base branch from main to feat/lw-9651-revamp-onboarding-develop-branch February 29, 2024 14:08
@greatertomi greatertomi changed the base branch from feat/lw-9651-revamp-onboarding-develop-branch to main February 29, 2024 14:11
@greatertomi greatertomi changed the base branch from main to feat/lw-9651-revamp-onboarding-develop-branch February 29, 2024 14:11
@greatertomi greatertomi merged commit 1418d66 into feat/lw-9651-revamp-onboarding-develop-branch Feb 29, 2024
9 of 10 checks passed
@greatertomi greatertomi deleted the feat/lw-9747-pin-extension branch February 29, 2024 14:11
greatertomi added a commit that referenced this pull request Mar 5, 2024
@greatertomi
feat: move pin extension notification to main screen (#902)
47906a0 
* feat: move pin extension notification to main screen

* fix: start notification timeout only when modal is close

* fix: notification useEffect improvement
emiride added a commit that referenced this pull request Mar 25, 2024
@greatertomi @emiride
@shawnbusuttil @oldGreg5 @danielmain
feat: revamp onboarding develop branch (#930)
57138c6 
* feat: move pin extension notification to main screen (#902)

* feat: move pin extension notification to main screen

* fix: start notification timeout only when modal is close

* fix: notification useEffect improvement

* feat: improve hardware wallet setup on onboarding flow (#909)

* feat(extension): remove beta dapp modal (#921)

* feat(extension): remove beta dapp modal

* test(extension): remove beta dapp modal

---------

Co-authored-by: emiride <[email protected]>

* feat: [LW-9740] remove legal and analytics page from onboarding flow (#890)

* feat: create confirmation banner

* feat: add analytics modal and landing page improvements

* feat: remove legal and analytics step on onboarding

* refactor: extract revamped screens from multi-wallet flow

* feat: [lw-9742] use new register screen for create wallet flow

* feat: [lw-9745]: revamp mnemonic verification

* fix: improvements

* [LW-9742]: use new register screen for create wallet flow (#891)

* feat: move pin extension notification to main screen (#902)

* feat: move pin extension notification to main screen

* fix: start notification timeout only when modal is close

* fix: notification useEffect improvement

* feat: improve hardware wallet setup on onboarding flow (#909)

* feat(extension): remove beta dapp modal (#921)

* feat(extension): remove beta dapp modal

* test(extension): remove beta dapp modal

---------

Co-authored-by: emiride <[email protected]>

* feat: [LW-9740] remove legal and analytics page from onboarding flow (#890)

* feat: create confirmation banner

* feat: add analytics modal and landing page improvements

* feat: remove legal and analytics step on onboarding

* refactor: extract revamped screens from multi-wallet flow

* feat: [lw-9742] use new register screen for create wallet flow (#891)

* feat: [lw-9745]: revamp mnemonic verification

* refactor: enable paste functionality for keyboard

* feat: adapt mnemonic length switcher to dark mode

* fix: styling and mnemonic pasting issues

* fix: remove recovery phrase length screen and clear clipboard after paste

* fix: focus automatically on first input field

* feat: show multi-address modal post wallet creation (#923)

* feat: show multi-address modal post wallet creation

* refactor: relocate multi-address modal

* fix: ensure pop-up only displays for multi-address wallet restoration

* Test/lw 9921 fix e2e tests (#934)

* feat: [lw-9742] use new register screen for create wallet flow

* feat: [LW-9740] remove legal and analytics page from onboarding flow (#890)

* feat: create confirmation banner

* feat: add analytics modal and landing page improvements

* feat: remove legal and analytics step on onboarding

* refactor: extract revamped screens from multi-wallet flow

* test(extension): lw-9921 fix e2e tests in onboarding

* test(extension): lw-9921 fix e2e tests in onboarding

* test(extension): onboarding fixes

* test(extension): fix try 1

* test(extension): fix try 2

* test(extension): fix try 3

* test(extension): fix try 4

* test(extension): cleanup

* test(extension): full fix 1

* test(extension): add new multi address modal confirm

---------

Co-authored-by: John Oshalusi <[email protected]>
Co-authored-by: januszjanus <[email protected]>

* fix(core): linting warning/errors

* fix: resolve onboarding create restore flow analytics (#936)

* fix: analytics in the wallet creation onboarding process

* fix: analytics in the wallet restore onboarding process

* feat: add analytics for wallet onboarding page

* feat: add analytics to multi-address modal

* fix(extension): fixed multi-wallet integration tests

* fix(extension): ignored test

* fix: design inconsistencies between screens

* test: fix AnalyticsTracker test

* feat: move the register screen after the mnemonic screen for create and restore flow (#945)

* feat: move the register screen after the mnemonic screen

* refactor: move WalletSetupNamePasswordStep translations into browser folder

* fix: more improvements

* feat: revamp forgot password flow (#956)

* feat: move the register screen after the mnemonic screen

* feat: revamp forgot password flow

* feat: move the register screen after the mnemonic screen

* fix(extension): fix eslint warning

* fix(extension): fix more eslint warnings where functions are not returning anything

* fix: remove back button on first step of forgot password

---------

Co-authored-by: John Oshalusi <[email protected]>
Co-authored-by: Daniel Main <[email protected]>
Co-authored-by: Emir Hodzic <[email protected]>

* feat: update save recovery phrase onboarding text (#966)

* fix: revert changes to multi-wallet onboarding flow (#965)

* fix: revert changes to multi-wallet onboarding flow

* Merge branch 'main' into feat/lw-9651-revamp-onboarding-develop-branch

* feat: handle back from next step in wallet setup step (#971)

* feat: handle back from next step in wallet setup step

* fix: remove unused variable from .env.developerpreview

* Feat/lw 9998 revamp e2e fixes (#949)

* test(extension): add new multi address modal confirm disabling

* test(extension): wip 1

* test(extension): wip 2

* test(extension): wip 3

* test(extension): add new multi address modal confirm disabling

* test(extension): wip 1

* test(extension): wip 2

* test(extension): fixed new smoke test

* test(extension): wip 4

* test(extension): password checks fix

* test(extension): conflicting commits sort out

* test(extension): add analytics annotation to analytics features

* test(extension): add e2e annotation to all e2e folder feature files

* test(extension): add onboarding annotation to all onboarding feature files

* test(extension): fix wallet address tests

* test(extension): fix forgot password tests

* test(extension): fixed test LW-4993

* test(extension): added test ids to mnemonic step

* test(extension): various fixes

* test(extension): fixed few create wallet tests

* test(extension): onboarding create tests fixes

* test(extension): onboarding create tests fixes 2

* test(extension): onboarding recovery tests fixes 1

* test(extension): fixes on restore wallet

* test(extension): contd fixing onboarding tests

* test(extension): fixed onboarding tests

* test(extension): fixed onboarding tests

* test(extension): additional refactoring

* test(extension): removed some unused steps

* test(extension): removed some unused steps

* test(extension): more fixes

* test(extension): more fixes 2

* test(extension): fixed new revamped tests

* test(extension): fixed smoke tests

* test(extension): refixed create and restore tests

* test(extension): smoke fix try 1

* test(extension): smoke fix try 2

* test(extension): smoke fix try 2

* test(extension): smoke fix try 3

* test(extension): smoke fix try 4

* test(extension): smoke fix try 5

* test(extension): smoke fix try 6

* test(extension): smoke fix try 7

* test(extension): smoke fix try 8

* test(extension): smoke fix try 8

* test(extension): smoke fix try 10

* test(extension): various fixes

* test(extension): various fixes 2

* test(extension): various fixes 3

* test(extension): smoke fix try

* test(extension): cleanup

* test(extension): 3761 fix try 1

* test(extension): cleanup 2

* test(extension): cleanup 3

* test(extension): pr comments

* test(extension): pr comments

* test(extension): pr comments, add analyticsBannerAssert

* test(extension): pr comments 2

* test(extension): few minor fixes

* test(extension): pr comments 3

* test(extension): fix on comments

* test(extension): cleanup

* test(extension): cleanup 2

---------

Signed-off-by: Emir Hodzic <[email protected]>
Co-authored-by: januszjanus <[email protected]>

* fix: update hardware wallet flow to better match revamped onboarding (#979)

* fix: update hardware wallet flow to match revamped onboarding

---------

Signed-off-by: John Oshalusi <[email protected]>
Signed-off-by: Emir Hodzic <[email protected]>
Co-authored-by: emiride <[email protected]>
Co-authored-by: Shawn <[email protected]>
Co-authored-by: shawnbusuttil <[email protected]>
Co-authored-by: januszjanus <[email protected]>
Co-authored-by: Daniel Main <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szymonmaslowski szymonmaslowski szymonmaslowski approved these changes

@shawnbusuttil shawnbusuttil shawnbusuttil approved these changes

@emiride emiride emiride approved these changes

@danielmain danielmain danielmain approved these changes

Assignees
No one assigned
Labels
browser Changes to the browser application.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@greatertomi @coveralls @szymonmaslowski @vhulchenko-iohk @danielmain @emiride @shawnbusuttil