Page MenuHomePhabricator
Differential D4786

[web] Introduce `AllNotifsIllustration` component to replace S3-hosted asset
ClosedPublic
Actions

Authored by atul on Aug 9 2022, 12:02 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 18, 1:03 AM
Unknown Object (File)
Sun, Mar 16, 10:34 AM
Unknown Object (File)
Sat, Mar 8, 6:42 AM
Unknown Object (File)
Sat, Mar 8, 6:42 AM
Unknown Object (File)
Sat, Mar 8, 6:41 AM
Unknown Object (File)
Sat, Mar 8, 6:37 AM
Unknown Object (File)
Feb 16 2025, 6:55 PM
Unknown Object (File)
Feb 3 2025, 11:58 AM
View All 86 Files
Subscribers
• adrian
ashoat

Details

Reviewers
varun
• abosh
tomek
• jacek
Commits
rCOMM2b24a6f0d704: [web] Introduce `AllNotifsIllustration` component to replace S3-hosted asset
Summary

Context: https://linear.app/comm/issue/ENG-1584/[web]-replace-s3-svg-urls-with-svgr-generated-react-components

We previously were fetching all-notifs.svg from S3. It took a second for the SVG to download, which led to the page reflowing. We got around the reflowing issue by explicitly stating the height/width of every SVG asset so there was some "placeholder space" left for the asset to fill once it loaded. This led to an expensive process when adding SVG assets to web.

Instead, we can convert the SVG into a React component using SVGR (https://react-svgr.com/playground/) and include it in the repo. This also lets us avoid fetching the asset once the app has loaded.

Depends on D4785

Test Plan

Things continue to look as expected.

Before:

Screen Shot 2022-08-09 at 3.00.54 PM.png (1×868 px, 97 KB)

After:

Screen Shot 2022-08-09 at 2.59.13 PM.png (1×868 px, 126 KB)

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Aug 9 2022, 12:02 PM
Herald added subscribers: • adrian, ashoat. · View Herald TranscriptAug 9 2022, 12:02 PM
atul edited the test plan for this revision. (Show Details)Aug 9 2022, 12:03 PM
Harbormaster completed remote builds in B11255: Diff 15490.Aug 9 2022, 12:12 PM
atul requested review of this revision.Aug 9 2022, 12:12 PM
atul added a child revision: D4787: [web] Introduce `BadgeNotifsIllustration` component to replace S3-hosted asset.Aug 9 2022, 12:16 PM
• abosh accepted this revision.Aug 9 2022, 12:19 PM
This revision is now accepted and ready to land.Aug 9 2022, 12:19 PM
atul added 1 blocking reviewer(s): varun.Aug 9 2022, 12:25 PM
This revision now requires review to proceed.Aug 9 2022, 12:25 PM
varun accepted this revision.Aug 9 2022, 12:39 PM

nice

This revision is now accepted and ready to land.Aug 9 2022, 12:39 PM
Closed by commit rCOMM2b24a6f0d704: [web] Introduce `AllNotifsIllustration` component to replace S3-hosted asset (authored by atul). · Explain WhyAug 10 2022, 7:52 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM2b24a6f0d704: [web] Introduce `AllNotifsIllustration` component to replace S3-hosted asset.
• abosh mentioned this in D4510: [landing] SIWE component with RainbowKit that connects & signs a message.Aug 11 2022, 12:10 PM

Revision Contents
Changeset List

PathSize
web/
7 lines
assets/
35 lines
modals/
threads/
notifications/
10 lines

Diff 15490

View Options

web/assets.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

web/assets/all-notifs.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

web/modals/threads/notifications/notifications-modal.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines