Page MenuHomePhabricator

[Cashtab] Deprecate getDeviceNotificationStyle
ClosedPublic

Authored by emack on Jan 30 2024, 13:58.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABCec4f96193676: [Cashtab] Deprecate getDeviceNotificationStyle
Summary

In the process of optimizing notifications it became apparent that getDeviceNotificationStyle was a fix for a legacy issue where notifications were clipping the top frame of the browser on mobile devices. This is no longer required as the top margin is now adequate without the need to set mobile specific marginTop values.

Test Plan

npm test
npm start
Trigger the following notifications across web, extension and mobile (iOS Safari and android chrome):

  • Send/Receive XEC
  • Send/Receive/Burn eToken
  • Create eToken and icon submission
  • Message signed / copied
  • Message/signature verified
  • Message/signature not verified
  • Register alias
  • Error

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

emack requested review of this revision.Jan 30 2024, 13:58

one of the bigger style changes here is that links are now blue in the txs.

imo acceptable as the notifications looked bad before and continue to look bad. something to improve in another diff.

green with nit -- rebase to latest master & version bump.

This revision is now accepted and ready to land.Jan 30 2024, 21:51

image.png (311×622 px, 28 KB)

In the future please include some before and after screenshots.

we are not practically able to test every one of these notifications on every single browser and device. it's also not worth the time to do something like this manually, especially on what is effectively a maintenance diff.

either we have integration tests that catch the important stuff automatically, like the content that renders inside a component. Or, if it's css only, screenshots + test for responsiveness is ok.

rebase and version bump