-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-8214] New Device Verification Notice UI #12360
Conversation
…lt based on flags, and can navigate back to vault after submission
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12360 +/- ##
==========================================
- Coverage 33.52% 33.50% -0.02%
==========================================
Files 2913 2921 +8
Lines 91086 91229 +143
Branches 17338 17358 +20
==========================================
+ Hits 30540 30570 +30
- Misses 58139 58251 +112
- Partials 2407 2408 +1 ☔ View full report in Codecov by Sentry. |
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! This is working as I'd expect, just a nit and a question.
if ( | ||
userItems?.last_dismissal == null && | ||
(userItems?.permanent_dismissal == null || !userItems?.permanent_dismissal) && | ||
(tempNoticeFlag || permNoticeFlag) | ||
) { |
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.
⛏️ (non-blocking) this logic could be cleaned up by deconstructing userItems
. The types don't suggest that userItems
can be null so I removed the optional chaining as well.
if ( | |
userItems?.last_dismissal == null && | |
(userItems?.permanent_dismissal == null || !userItems?.permanent_dismissal) && | |
(tempNoticeFlag || permNoticeFlag) | |
) { | |
const { last_dismissal, permanent_dismissal } = userItems; | |
if (last_dismissal == null && !permanent_dismissal && (tempNoticeFlag || permNoticeFlag)) { |
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 have this code altered in my following work so this is does not need to be changed here.
...onents/new-device-verification-notice/new-device-verification-notice-page-one.component.html
Show resolved
Hide resolved
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.
❓ Is this needed?
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.
Removed: 78fd445
…214-new-device-notice-UI
…214-new-device-notice-UI
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.
🎨 Should the icon use tailwind for fill to support light/dark mode?
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.
Updated for both icons: 54d872a
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 pretty good to me, just two questions before approval (and a few non-blocking nits)
...mponents/new-device-verification-notice/new-device-verification-notice-page-two.component.ts
Outdated
Show resolved
Hide resolved
...onents/new-device-verification-notice/new-device-verification-notice-page-two.component.html
Outdated
Show resolved
Hide resolved
...mponents/new-device-verification-notice/new-device-verification-notice-page-one.component.ts
Outdated
Show resolved
Hide resolved
...mponents/new-device-verification-notice/new-device-verification-notice-page-one.component.ts
Outdated
Show resolved
Hide resolved
- an empty href is needed so the links are keyboard accessible
- No noticeable difference in the end result
🎟️ Tracking
Browser UI
Web and Desktop
📔 Objective
Create the components and design for all three clients.
vault
routestwo step login
oraccount email
buttons will navigate to those settings pagesNOTE: This PR implements the designs, full guard and state retention logic will be added afterwards here PM-14347
📸 Screenshots
Screen Recording of Web Flow
https://github.com/user-attachments/assets/fa4074f0-debc-4aae-9411-12550d311f3b
Screen Recording of Browser Flow
https://github.com/user-attachments/assets/c643c02c-56c4-4e82-9461-ec8052191b14
Screen Recording of Browser Navigating to Settings
https://github.com/user-attachments/assets/669e18e7-fba0-4041-9647-2b057f39914a
Screenshots of Desktop
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes