Page MenuHomePhabricator
Differential D5452

[native] removed tooltip location logic
AbandonedPublic
Actions

Authored by ginsu on Oct 20 2022, 3:12 PM.
Tags
None
Referenced Files
F6236579: D5452.diff
Thu, Apr 24, 3:37 PM
Unknown Object (File)
Sun, Apr 13, 12:16 AM
Unknown Object (File)
Wed, Apr 9, 3:45 AM
Unknown Object (File)
Wed, Apr 9, 3:45 AM
Unknown Object (File)
Wed, Apr 9, 3:45 AM
Unknown Object (File)
Wed, Apr 9, 3:45 AM
Unknown Object (File)
Fri, Apr 4, 3:10 AM
Unknown Object (File)
Thu, Apr 3, 6:21 AM
View All Files
Subscribers
• abosh
ashoat
atul
tomek

Details

Reviewers
atul
rohan
ashoat
Summary

Will not land this diff until the next diff in the stack is accepted

removed the tooltip location logic since eventually, the tooltip toolbar will always be at the bottom of the chat window

Depends on D5451
Linear Task: ENG-2037

Test Plan

Please review the video demo to see that the tooltip component is now always going below the message:

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu created this revision.Oct 20 2022, 3:12 PM
Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald TranscriptOct 20 2022, 3:12 PM
ginsu edited the summary of this revision. (Show Details)Oct 20 2022, 3:16 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, rohan.
ginsu added a parent revision: D5451: [native] deduped open thread and create thread option.
ginsu attached a referenced file: F205235: Screen Recording 2022-10-20 at 6.13.46 PM.mov. (Show Details)
ginsu edited the summary of this revision. (Show Details)
ginsu edited the summary of this revision. (Show Details)
Harbormaster completed remote builds in B12954: Diff 17795.Oct 20 2022, 3:20 PM
ginsu requested review of this revision.Oct 20 2022, 3:20 PM
ashoat added inline comments.Oct 20 2022, 7:37 PM
native/navigation/tooltip.react.js
315 ↗(On Diff #17795)

I wonder if it's worth keeping this feature in Tooltip just in case we want to use it in the future. @atul, what do you think?

ginsu added a child revision: D5459: [native] moved tooltip toolbar to the bottom of the chat window.Oct 21 2022, 12:45 PM
atul added a comment.EditedOct 24 2022, 7:00 AM

@atul, what do you think?

I don't think it hurts to keep this positioning logic around. It might be helpful if we use Tooltip for the row of emoji reactions?

It looks like we have designs in Figma for the row of emoji to appear both above and below the message bubble:

aaac82.png (1×1 px, 178 KB)

ashoat requested changes to this revision.Oct 24 2022, 7:59 AM

Cool, agree

This revision now requires changes to proceed.Oct 24 2022, 7:59 AM
ginsu updated this revision to Diff 17857.Oct 24 2022, 8:31 AM

rebase

ginsu planned changes to this revision.Oct 24 2022, 8:35 AM

planning changes for addressing feedback

Harbormaster completed remote builds in B12998: Diff 17857.Oct 24 2022, 8:44 AM
ginsu abandoned this revision.Oct 24 2022, 9:15 AM

after speaking to @atul we came up with a better strategy of instead of removing the location logic, adding a fixed option to the location logic that fixes the toolbar to the bottom of the chat window

Revision Contents
Changeset List

PathSize
native/
chat/
5 lines
5 lines
5 lines
navigation/
43 lines

Diff 17857

View Options

native/chat/multimedia-message.react.js

Show First 20 Lines • Show All 311 Lines • ▼ Show 20 Lines (navigationState: ?PossiblyStaleNavigationState) => {
// Finally, we want to disable the swipe gesture if there is a stack with // Finally, we want to disable the swipe gesture if there is a stack with
// more than one subroute, since then the stack will have its own swipe // more than one subroute, since then the stack will have its own swipe
// gesture that will conflict with the drawer's // gesture that will conflict with the drawer's
return currentTabSubrouteState.routes.length < 2; return currentTabSubrouteState.routes.length < 2;
}, },
); );
const getChatNavState: (
navigationState: ?PossiblyStaleNavigationState,
) => ?PossiblyStaleNavigationState = (
navigationState: ?PossiblyStaleNavigationState,
) => {
tomekUnsubmitted
Done
Inline Actions
},
);
- const getChatNavState: (
- navigationState: ?PossiblyStaleNavigationState,
- ) => ?PossiblyStaleNavigationState = (
- navigationState: ?PossiblyStaleNavigationState,
- ) => {
+ function getChatNavState(navigationState: ?PossiblyStaleNavigationState): ?PossiblyStaleNavigationState {
if (!navigationState) {

It might be more readable using function syntax

tomek: It might be more readable using `function` syntax
if (!navigationState) {
return null;
}
const [firstAppSubroute] = navigationState.routes;
if (firstAppSubroute.name !== CommunityDrawerNavigatorRouteName) {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Usually the first route is the AppRouteName. Why is this different?

inka: Usually the first route is the `AppRouteName`. Why is this different?
kubaAuthorUnsubmitted
Done
Inline Actions

It is called from`OverlayRouter`, so it is nested.

kuba: It is called from`OverlayRouter`, so it is nested.
const communityDrawerState = getStateFromNavigatorRoute(firstAppSubroute);
const [firstCommunityDrawerSubroute] = communityDrawerState.routes;
if (firstCommunityDrawerSubroute.name !== TabNavigatorRouteName) {
return null;
}
const tabState = getStateFromNavigatorRoute(firstCommunityDrawerSubroute);
if (!tabState) {
return null;
}
let chatRoute;
for (const route of tabState.routes) {
if (route.name === ChatRouteName) {
chatRoute = route;
break;
}
}
if (!chatRoute || !chatRoute.state) {
return null;
}
const chatRouteState = getStateFromNavigatorRoute(chatRoute);
if (chatRouteState.type !== 'stack') {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Why is this necessary? When can chatRouteState.type not be 'stack'? Or is this something flow requires?

inka: Why is this necessary? When can `chatRouteState.type` not be `'stack'`? Or is this something…
kubaAuthorUnsubmitted
Done
Inline Actions

This function is based on drawerSwipeEnabledSelector function. It might be unnecessary. Not sure if I should remove it.

kuba: This function is based on `drawerSwipeEnabledSelector` function. It might be unnecessary. Not…
return chatRouteState;
};
const getRemoveEditMode: ( const getRemoveEditMode: (
chatRouteState: ?PossiblyStaleNavigationState, chatRouteState: ?PossiblyStaleNavigationState,
) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => { ) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => {
if (!chatRouteState) { if (!chatRouteState) {
return null; return null;
} }
const messageListRoute = const messageListRoute =
chatRouteState.routes[chatRouteState.routes.length - 1]; chatRouteState.routes[chatRouteState.routes.length - 1];
Show All 29 Linesexport {
useActiveMessageList, useActiveMessageList,
calendarActiveSelector, calendarActiveSelector,
nativeCalendarQuery, nativeCalendarQuery,
nonThreadCalendarQuery, nonThreadCalendarQuery,
useCalendarQuery, useCalendarQuery,
drawerSwipeEnabledSelector, drawerSwipeEnabledSelector,
useCurrentLeafRouteName, useCurrentLeafRouteName,
getRemoveEditMode, getRemoveEditMode,
getChatNavState,
}; };
View Options

native/chat/robotext-message.react.js

Show First 20 Lines • Show All 311 Lines • ▼ Show 20 Lines (navigationState: ?PossiblyStaleNavigationState) => {
// Finally, we want to disable the swipe gesture if there is a stack with // Finally, we want to disable the swipe gesture if there is a stack with
// more than one subroute, since then the stack will have its own swipe // more than one subroute, since then the stack will have its own swipe
// gesture that will conflict with the drawer's // gesture that will conflict with the drawer's
return currentTabSubrouteState.routes.length < 2; return currentTabSubrouteState.routes.length < 2;
}, },
); );
const getChatNavState: (
navigationState: ?PossiblyStaleNavigationState,
) => ?PossiblyStaleNavigationState = (
navigationState: ?PossiblyStaleNavigationState,
) => {
tomekUnsubmitted
Done
Inline Actions
},
);
- const getChatNavState: (
- navigationState: ?PossiblyStaleNavigationState,
- ) => ?PossiblyStaleNavigationState = (
- navigationState: ?PossiblyStaleNavigationState,
- ) => {
+ function getChatNavState(navigationState: ?PossiblyStaleNavigationState): ?PossiblyStaleNavigationState {
if (!navigationState) {

It might be more readable using function syntax

tomek: It might be more readable using `function` syntax
if (!navigationState) {
return null;
}
const [firstAppSubroute] = navigationState.routes;
if (firstAppSubroute.name !== CommunityDrawerNavigatorRouteName) {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Usually the first route is the AppRouteName. Why is this different?

inka: Usually the first route is the `AppRouteName`. Why is this different?
kubaAuthorUnsubmitted
Done
Inline Actions

It is called from`OverlayRouter`, so it is nested.

kuba: It is called from`OverlayRouter`, so it is nested.
const communityDrawerState = getStateFromNavigatorRoute(firstAppSubroute);
const [firstCommunityDrawerSubroute] = communityDrawerState.routes;
if (firstCommunityDrawerSubroute.name !== TabNavigatorRouteName) {
return null;
}
const tabState = getStateFromNavigatorRoute(firstCommunityDrawerSubroute);
if (!tabState) {
return null;
}
let chatRoute;
for (const route of tabState.routes) {
if (route.name === ChatRouteName) {
chatRoute = route;
break;
}
}
if (!chatRoute || !chatRoute.state) {
return null;
}
const chatRouteState = getStateFromNavigatorRoute(chatRoute);
if (chatRouteState.type !== 'stack') {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Why is this necessary? When can chatRouteState.type not be 'stack'? Or is this something flow requires?

inka: Why is this necessary? When can `chatRouteState.type` not be `'stack'`? Or is this something…
kubaAuthorUnsubmitted
Done
Inline Actions

This function is based on drawerSwipeEnabledSelector function. It might be unnecessary. Not sure if I should remove it.

kuba: This function is based on `drawerSwipeEnabledSelector` function. It might be unnecessary. Not…
return chatRouteState;
};
const getRemoveEditMode: ( const getRemoveEditMode: (
chatRouteState: ?PossiblyStaleNavigationState, chatRouteState: ?PossiblyStaleNavigationState,
) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => { ) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => {
if (!chatRouteState) { if (!chatRouteState) {
return null; return null;
} }
const messageListRoute = const messageListRoute =
chatRouteState.routes[chatRouteState.routes.length - 1]; chatRouteState.routes[chatRouteState.routes.length - 1];
Show All 29 Linesexport {
useActiveMessageList, useActiveMessageList,
calendarActiveSelector, calendarActiveSelector,
nativeCalendarQuery, nativeCalendarQuery,
nonThreadCalendarQuery, nonThreadCalendarQuery,
useCalendarQuery, useCalendarQuery,
drawerSwipeEnabledSelector, drawerSwipeEnabledSelector,
useCurrentLeafRouteName, useCurrentLeafRouteName,
getRemoveEditMode, getRemoveEditMode,
getChatNavState,
}; };
View Options

native/chat/text-message.react.js

Show First 20 Lines • Show All 311 Lines • ▼ Show 20 Lines (navigationState: ?PossiblyStaleNavigationState) => {
// Finally, we want to disable the swipe gesture if there is a stack with // Finally, we want to disable the swipe gesture if there is a stack with
// more than one subroute, since then the stack will have its own swipe // more than one subroute, since then the stack will have its own swipe
// gesture that will conflict with the drawer's // gesture that will conflict with the drawer's
return currentTabSubrouteState.routes.length < 2; return currentTabSubrouteState.routes.length < 2;
}, },
); );
const getChatNavState: (
navigationState: ?PossiblyStaleNavigationState,
) => ?PossiblyStaleNavigationState = (
navigationState: ?PossiblyStaleNavigationState,
) => {
tomekUnsubmitted
Done
Inline Actions
},
);
- const getChatNavState: (
- navigationState: ?PossiblyStaleNavigationState,
- ) => ?PossiblyStaleNavigationState = (
- navigationState: ?PossiblyStaleNavigationState,
- ) => {
+ function getChatNavState(navigationState: ?PossiblyStaleNavigationState): ?PossiblyStaleNavigationState {
if (!navigationState) {

It might be more readable using function syntax

tomek: It might be more readable using `function` syntax
if (!navigationState) {
return null;
}
const [firstAppSubroute] = navigationState.routes;
if (firstAppSubroute.name !== CommunityDrawerNavigatorRouteName) {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Usually the first route is the AppRouteName. Why is this different?

inka: Usually the first route is the `AppRouteName`. Why is this different?
kubaAuthorUnsubmitted
Done
Inline Actions

It is called from`OverlayRouter`, so it is nested.

kuba: It is called from`OverlayRouter`, so it is nested.
const communityDrawerState = getStateFromNavigatorRoute(firstAppSubroute);
const [firstCommunityDrawerSubroute] = communityDrawerState.routes;
if (firstCommunityDrawerSubroute.name !== TabNavigatorRouteName) {
return null;
}
const tabState = getStateFromNavigatorRoute(firstCommunityDrawerSubroute);
if (!tabState) {
return null;
}
let chatRoute;
for (const route of tabState.routes) {
if (route.name === ChatRouteName) {
chatRoute = route;
break;
}
}
if (!chatRoute || !chatRoute.state) {
return null;
}
const chatRouteState = getStateFromNavigatorRoute(chatRoute);
if (chatRouteState.type !== 'stack') {
return null;
}
inkaUnsubmitted
Not Done
Inline Actions

Why is this necessary? When can chatRouteState.type not be 'stack'? Or is this something flow requires?

inka: Why is this necessary? When can `chatRouteState.type` not be `'stack'`? Or is this something…
kubaAuthorUnsubmitted
Done
Inline Actions

This function is based on drawerSwipeEnabledSelector function. It might be unnecessary. Not sure if I should remove it.

kuba: This function is based on `drawerSwipeEnabledSelector` function. It might be unnecessary. Not…
return chatRouteState;
};
const getRemoveEditMode: ( const getRemoveEditMode: (
chatRouteState: ?PossiblyStaleNavigationState, chatRouteState: ?PossiblyStaleNavigationState,
) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => { ) => ?RemoveEditMode = (chatRouteState: ?PossiblyStaleNavigationState) => {
if (!chatRouteState) { if (!chatRouteState) {
return null; return null;
} }
const messageListRoute = const messageListRoute =
chatRouteState.routes[chatRouteState.routes.length - 1]; chatRouteState.routes[chatRouteState.routes.length - 1];
Show All 29 Linesexport {
useActiveMessageList, useActiveMessageList,
calendarActiveSelector, calendarActiveSelector,
nativeCalendarQuery, nativeCalendarQuery,
nonThreadCalendarQuery, nonThreadCalendarQuery,
useCalendarQuery, useCalendarQuery,
drawerSwipeEnabledSelector, drawerSwipeEnabledSelector,
useCurrentLeafRouteName, useCurrentLeafRouteName,
getRemoveEditMode, getRemoveEditMode,
getChatNavState,
}; };
View Options

native/navigation/tooltip.react.js

Loading...