Page MenuHomePhabricator
Differential D5491

[native] Added highlight effect to arrow background
ClosedPublic
Actions

Authored by • przemek on Oct 27 2022, 6:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 2, 1:31 PM
Unknown Object (File)
Wed, Apr 2, 12:09 PM
Unknown Object (File)
Wed, Apr 2, 12:08 PM
Unknown Object (File)
Wed, Apr 2, 9:44 AM
Unknown Object (File)
Wed, Apr 2, 8:38 AM
Unknown Object (File)
Wed, Apr 2, 3:25 AM
Unknown Object (File)
Wed, Apr 2, 2:31 AM
Unknown Object (File)
Tue, Apr 1, 8:01 AM
View All Files
Subscribers
• abosh
ashoat
atul
bartek
ginsu
kamil
tomek

Details

Reviewers
tomek
kamil
atul
bartek
ginsu
Commits
rCOMMf0745ad28684: [native] Added highlight effect to arrow background
Summary

Solving this: https://linear.app/comm/issue/ENG-2045/add-highlight-effect-to-arrow-background-when-finger-is-on-thread-row

Reorganised SidebarListModal, ChatThreadListSidebar and SidebarItem, so that
SidebarItem doesn't hold button anymore, but rather is wrapped inside it.
Changed arrows SVG, so instead of short and long ones we have arrows extending in both ways and
short arrows that don't extend downwards, so they can finish list of thread sidebars.

Test Plan

Build app in iOS and Android simulator.
Performed extensive visual testing if component is rendered correctly.

Diff Detail

Repository
rCOMM Comm
Branch
fix/row-highlight
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• przemek created this revision.Oct 27 2022, 6:52 AM
Herald added a subscriber: • abosh. · View Herald TranscriptOct 27 2022, 6:52 AM
• przemek edited the summary of this revision. (Show Details)Oct 27 2022, 6:52 AM
• przemek added 1 blocking reviewer(s): tomek.
• przemek attached a referenced file: F208809: Screen Recording 2022-10-27 at 14.58.32.mov. (Show Details)
Harbormaster completed remote builds in B13069: Diff 17944.Oct 27 2022, 7:02 AM
• przemek requested review of this revision.Oct 27 2022, 7:02 AM
ginsu requested changes to this revision.Oct 27 2022, 7:27 AM
ginsu added a subscriber: ginsu.

The highlight effect looks great! However, we want to make sure that when the user swipes on a thread, the arrows don't break. Here is the Figma doc for more context

This revision now requires changes to proceed.Oct 27 2022, 7:27 AM
• przemek updated this revision to Diff 17958.EditedOct 27 2022, 9:14 AM

Right, put SwipeableThread inside of Button now, so it is handled properly

• przemek attached a referenced file: Unknown Object (File). (Show Details)Oct 27 2022, 9:15 AM
• przemek attached a referenced file: F209086: recodding.mov. (Show Details)Oct 27 2022, 9:20 AM
Harbormaster completed remote builds in B13081: Diff 17958.Oct 27 2022, 9:28 AM
bartek accepted this revision.Oct 28 2022, 12:35 AM
tomek resigned from this revision.Oct 28 2022, 4:01 AM

The result looks great!
@kamil please review and add me after accepting.

ginsu accepted this revision.Oct 28 2022, 6:57 AM

nice, looks good! thank you for fixing that swipe effect

This revision is now accepted and ready to land.Oct 28 2022, 6:57 AM
kamil accepted this revision.Nov 2 2022, 3:45 AM
kamil added 1 blocking reviewer(s): tomek.
This revision now requires review to proceed.Nov 2 2022, 3:45 AM
tomek requested changes to this revision.Nov 2 2022, 10:04 AM
tomek added inline comments.
native/chat/chat-thread-list-sidebar.react.js
97

This is the default value

native/chat/sidebar-list-modal.react.js
85

We should use callback here

99

We don't need to depend on the whole value when we use only a part of it

This revision now requires changes to proceed.Nov 2 2022, 10:04 AM
• przemek updated this revision to Diff 18049.EditedNov 3 2022, 4:40 AM
• przemek marked 2 inline comments as done.

Responded to inline comments. Tries to properly use useCallback resulted in creating extra small component Item added in sidebar-item.react.js.
Spotted a visual bug Swipeable row's height was smaller then height of wrapping component (you can observed it in video above when I swipe threads - green and red buttons do not fill entire height). It should be fixed now.

• przemek attached a referenced file: F218750: Screen Recording 2022-11-03 at 12.35.14.mov. (Show Details)Nov 3 2022, 4:40 AM
• przemek marked an inline comment as done.
• przemek added inline comments.
native/chat/chat-thread-list-sidebar.react.js
97

Didn't know it. Deleted all explicit display: 'flex' from those components

Harbormaster completed remote builds in B13152: Diff 18049.Nov 3 2022, 4:54 AM
tomek accepted this revision.Nov 4 2022, 6:07 AM
tomek added inline comments.
native/chat/sidebar-list-modal.react.js
118 ↗(On Diff #18049)

There's no need now to accept inexact props - we can specify what we actually need and let renderItem provide only these. In this case, instead of providing a row, we need just item.

This revision is now accepted and ready to land.Nov 4 2022, 6:07 AM
• przemek updated this revision to Diff 18073.Nov 4 2022, 6:21 AM

Fixed Item props.

Harbormaster completed remote builds in B13169: Diff 18073.Nov 4 2022, 6:34 AM
Closed by commit rCOMMf0745ad28684: [native] Added highlight effect to arrow background (authored by • przemek). · Explain WhyNov 4 2022, 6:38 AM
This revision was automatically updated to reflect the committed changes.
pweglik <przemek.weglik@onet.pl> added a commit: rCOMMf0745ad28684: [native] Added highlight effect to arrow background.

Revision Contents
Changeset List

Diff 17958

View Options

native/chat/chat-thread-list-item.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

native/chat/chat-thread-list-sidebar.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

native/chat/sidebar-item.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

native/chat/sidebar-list-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
View Options

native/components/arrow-extended.react.js

Loading...
View Options

native/components/arrow-long.react.js

Loading...