[Cashtab] Change notification position on mobile
ClosedPublic
Actions

Authored by kieran709 on Dec 7 2021, 19:38.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Commits

rABC1de6e2c93142: [Cashtab] Change notification position on mobile

Summary

Transaction notifications were not legible in some mobile views. Function added to notifications.js to detect whether user is on mobile or desktop, and sets the notification's style accordingly. On mobile, notification now has margin-top: 10% to make it legible if the device is a mobile. Related to task T1889

Test Plan

cd web/cashtab && npm start
Inspect and toggle device to mobile in chrome
send XEC and ensure notification is pushed lower in the viewport
create token and ensure notification is pushed lower in viewport
submit token icon and ensure notification is pushed lower in viewport
send token and ensure notification is pushed lower in viewport
receive XEC and ensure notification is pushed lower in viewport
receive eToken and ensure notification is pushed lower in viewport
sign message and ensure notification is pushed lower in viewport
force error and ensure notification is pushed lower in viewport

Diff Detail

Repository

rABC Bitcoin ABC

Branch

notification-placement-mobile

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17581
Build 34988: Build Diff	cashtab-tests
Build 34987: arc lint + arc unit

Event Timeline

kieran709 created this revision.Dec 7 2021, 19:38

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 7 2021, 19:38

kieran709 requested review of this revision.Dec 7 2021, 19:38

Still working through some of the tests, will update status once finished.

Harbormaster completed remote builds in B17566: Diff 31307.Dec 7 2021, 19:41

Testing complete, ready for review.

bytesofman requested changes to this revision.Dec 8 2021, 15:08

bytesofman added inline comments.

web/cashtab/src/components/Common/Notifications.js
12 ↗	(On Diff #31307)	We don't want this to be a globally defined variable for this file, delete this line
14 ↗	(On Diff #31307)	I think this function name could be improved, say `getDeviceNotificationStyle()`
31 ↗	(On Diff #31307)	This should read `const notificationStyle = identifyDeviceAndSetNotificationStyle()` in all instances

This revision now requires changes to proceed.Dec 8 2021, 15:08

Removed globally defined variable, updated function name and changed function call per review feedback.

Harbormaster completed remote builds in B17581: Diff 31326.Dec 8 2021, 16:19

1st level review is green - tested all ok on iOS and Android devices.
Linking D10667 here as a reminder to implement these visual enhancements for the NFT notifications as well down the track.

bytesofman accepted this revision.Dec 13 2021, 19:35

This revision is now accepted and ready to land.Dec 13 2021, 19:35

Closed by commit rABC1de6e2c93142: [Cashtab] Change notification position on mobile (authored by kieran709). · Explain WhyDec 13 2021, 20:47

This revision was automatically updated to reflect the committed changes.

kieran709 added a commit: rABC1de6e2c93142: [Cashtab] Change notification position on mobile.

kieran709 edited the summary of this revision. (Show Details)Dec 13 2021, 20:57