Closed kavimuru closed 1 year ago
Triggered auto assignment to @greg-schroeder (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External
)
Current assignee @roryabraham is eligible for the External assigner, not assigning anyone new.
When you sign-in you will get a spinner not a blank page, and I think this is the expected behaviour. Not sure what's the problem here
Auto-assign attempt failed, all eligible assignees are OOO.
Tagging in design for precise mockups.
When you sign-in you will get a spinner not a blank page, and I think this is the expected behaviour. Not sure what's the problem here
We have a "skeleton UI" component that shows a skeleton of UI components, and that's our preferred UI pattern over a full-screen loading spinner. This is technically a new feature, not a bug (though I'd argue it just implements our established best practice on another important screen). I'm sure that the distinction will become clear once we have mockups. However, let's all be patient here because @shawnborton is currently our only designer, and this issue might not be as urgent as others he may be assigned to.
There are 3 cases after login and before loading LHN:
full loading page
LHN and Report detail page is displayed and all are blank
LHN and Report detail page is displayed and only FAB icon shows on LHN
on my iPhone 14 Pro the FAB appears as barely visible in the top right of the screen
I am not able to reproduce now but I have ever experienced this in the past on my iPhone @roryabraham It would be helpful if you can add screenshot after reproduction
To clarify, would we get rid of the full page loading spinner in lieu of just showing skeleton UI components?
As far as the mockup goes, I guess we'd have something like this?
Yep, that looks great!
To clarify, would we get rid of the full page loading spinner in lieu of just showing skeleton UI components?
Great question. Why wouldn't we replace the spinner with the skeleton UI more broadly? It seems like a pretty good change all around, and in fact, we're also doing this in RHP as well.
Why wouldn't we replace the spinner with the skeleton UI more broadly?
I think we should – that's what this issue is all about. But we can just take it step-by-step
I commented in the other issue but I feel like the skeleton loader makes sense when we are about to load messages or chats. Alternatively, we could create a skeleton that mimics the exact layout of each page that we need to load. So for instance, if we need to load the workspace page, we could have a skeleton that looks like the big avatar image, plus some text, plus option rows.
Alternatively, we could create a skeleton that mimics the exact layout of each page that we need to load
Love this idea
Yeah, I love that as well. Eventually. It does feel more like a polish initiative though.
@JmillsExpensify let's bump the bounty for this issue up to $1000 to see if we can get some proposals coming in!
@CosminnB Overall that looks pretty good, but you lost me a bit with this statement:
As an add-on to the existing SkeletonViewLines component we can add a platform-specific condition as the react-content-loader library will not properly show the animation on web.
Can you expand on what you mean? And why that isn't a problem with our current implementation of the skeleton view for reports?
I don't think this is correct design we're looking for. It's weird that left side shows skeleton, while right side doesn't show anything.
As explained in mockup @shawnborton attached here, show skeleton on both sides (LHN and Report page) until each of them are fully loaded
Please correct me if I am wrong cc: @roryabraham @shawnborton
Yeah, I think if we are loading a chat over in the main chat view, we should show the skeleton there too.
@shawnborton, @greg-schroeder, @roryabraham, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@JmillsExpensify let's bump the bounty for this issue up to $1000 to see if we can get some proposals coming in!
@roryabraham Do you want me to assign myself to this issue? I'll go ahead and bump it up to $1,000 though heads up that @greg-schroeder is the BZ member assigned. Happy to help though!
@CosminnB we have full page loading spinner after login, before loading main page. Please understand this first. (above screenshot is for Step 1)
Looks like something related to react-navigation
may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js
files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
@CosminnB The full loading page is 100% displayed every time for me. Please try with HT account.
my proposal removes it in favor of showing the skeleton
I don't see any code snippet in your proposal related to this.
@CosminnB I followed and tested your updated proposal but still showing blank page on LHN instead of skeleton after login. To ensure that I am not missing anything from your proposal, can you create your branch and share link?
Also, next time, please post new proposal instead of editing previous (deprecated) proposal.
Sounds like we're still waiting for a functional proposal here
@CosminnB did you test yourself on that branch? please check this weird behavior
Next time when you submit updated proposal,
main
We're experiencing auth 403 error in web dev right now (conversation here). Will review again once that is fixed. However, still waiting for better proposals.
@aimane-chnaif Any update here?
About your second point, you can't really keep everything in one commit and also be in sync with the main branch because of the hundreds of commits coming in every day.
@CosminnB
If you can't put everything in one commit, just post proposal with code changes as usual.
I am not asking you to be in sync always but at least you can pull from main
when re-request review. Your current branch is too old.
As you see from your video, it goes to "no access" page after login. This shouldn't happen. That's the weird behavior I mentioned.
@aimane-chnaif Any update here?
still waiting for proposals
Not overdue, sounds like we're still waiting for proposals.
Proposal
The full-page loading comes from MainDrawerNavigator
where we show it while waiting for reports to be loaded before mounting the drawer navigator. Let's remove it. Now, when we mount the navigator while the reports are not ready yet, the initialParams.reportID
will be an empty string and we need a way to update the initialParams
. To update it, we can use Navigaton.setParams
which will be a new function to update the current route params.
MainDrawerNavigator.js
shouldComponentUpdate(nextProps) {
const initialNextParams = getInitialReportScreenParams(nextProps.reports, !Permissions.canUseDefaultRooms(nextProps.betas), nextProps.policies);
if (this.initialParams.reportID === initialNextParams.reportID) {
return false;
}
+ if (!this.initialParams.reportID) {
+ Navigation.setParams(initialNextParams);
+ }
this.initialParams = initialNextParams;
return true;
}
render() {
- // Wait until reports are fetched and there is a reportID in initialParams
- if (!this.initialParams.reportID) {
- return <FullScreenLoadingIndicator logDetail={{name: 'Main Drawer Loader', initialParams: this.initialParams}} />;
- }
- // After the app initializes and reports are available the home navigation is mounted
- // This way routing information is updated (if needed) based on the initial report ID resolved.
- // This is usually needed after login/create account and re-launches
return (
<BaseDrawerNavigator
drawerContent={({navigation, state}) => {
// This state belongs to the drawer so it should always have the ReportScreen as it's initial (and only) route
const reportIDFromRoute = lodashGet(state, ['routes', 0, 'params', 'reportID']);
return (
<SidebarScreen
navigation={navigation}
reportIDFromRoute={reportIDFromRoute}
/>
);
}}
screens={[
{
name: SCREENS.REPORT,
component: ReportScreen,
initialParams: this.initialParams,
},
]}
isMainScreen
/>
);
}
Navigation.js
/**
* Update route params for the currently focused route.
*
* @param {Object} params
*/
function setParams(params) {
navigationRef.current.dispatch(CommonActions.setParams(params));
}
Because we are mounting the drawer navigator (which contains sidebar and report screen) possibly before reports are loaded, we need to handle the emptiness of some data.
First, the sidebar screen.
On SidebarLinks.js
, we return null
when the personalDetails
is empty. We can remove it to prevent a blank page. Now, because we are removing it, we are showing the sidebar header which contains search and settings icon, which could lead to some bugs (opening the search page while reports/personal details are empty shows an empty list, we don't know what could happen when someone accesses the settings while reports/personal details are not ready). Maybe we need to show a skeleton on the header too? Or hide both?
We also have optionListItems
which could be empty, and we need to show the skeleton.
As we don't have a skeleton view for the LHN list, we need to make a new one. The code is inspired by the existing skeleton view component.
const LHNSkeletonView = props => {
const skeletonHeight = CONST.CHAT_SKELETON_VIEW.HEIGHT_FOR_ROW_COUNT[1];
const possibleVisibleContentItems = Math.ceil(props.containerHeight / skeletonHeight);
const skeletonViewLines = [];
for (let index = 0; index < possibleVisibleContentItems; index++) {
const lengthIndex = index % 3;
let lineWidth;
switch (lengthIndex) {
case 0:
lineWidth = "100%";
break;
case 1:
lineWidth = "50%";
break;
default:
lineWidth = "65%";
}
skeletonViewLines.push(
<SkeletonViewContentLoader
key={`skeletonViewLines${index}`}
animate={props.animate}
height={skeletonHeight}
backgroundColor={themeColors.appBG}
foregroundColor={themeColors.border}
style={styles.mr5}
>
<Circle cx="40" cy="26" r="20" />
<Rect x="67" y="11" width="20%" height="8" />
<Rect x="67" y="31" width={lineWidth} height="8" />
</SkeletonViewContentLoader>
)
}
return <>{skeletonViewLines}</>;
};
I use a different color for the background because the color used on the report actions skeleton is the same as the sidebar background color.
And use it as the empty list component in LHNOptionsList
<FlatList
...
- onLayout={this.props.onLayout}
+ onLayout={(event) => {
+ this.props.onLayout();
+ const flatListHeight = event.nativeEvent.layout.height;
+
+ // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
+ // takes up so we can set the skeleton view container height.
+ if (flatListHeight === 0) {
+ return;
+ }
+ this.setState({flatListHeight});
+ }}
+ ListEmptyComponent={<LHNSkeletonView containerHeight={this.state.flatListHeight} />}
/>
Sidebar done. The last part is the report screen.
First of all, because now we are receiving an empty reportID, when we log in, the report screen is opened and reportActions
is loaded with a key of reportActions_
, it will return an object with the last key as the object key and have null
value (e.g. {reportActions_[id]: null}
) which is not expected as it will give a false
result at this condition which results into the ReportActionsView
mounted with null
reportActions. (Except when we clear the app data before logging in)
We have 2 ways to solve this.
First, we can return "-1" when the reportID
is empty. Doing this, the initial reportActions
will always be an empty object.
function getReportID(route) {
- return route.params.reportID.toString();
+ return route.params.reportID.toString() || '-1';
}
Second, we can add another check to reportActions
by loop over the keys and check if it's null.
Now, in ReportScreen
, we already have a skeleton view that will be shown when freeze
is true. Similar to the sidebar, there is also a check which will return null
. We can also remove it and move the condition to freeze
.
- const freeze = this.props.isSmallScreenWidth && this.props.isDrawerOpen
+ const freeze = (this.props.isSmallScreenWidth && this.props.isDrawerOpen) || (reportID === '-1' || !this.props.isSidebarLoaded || _.isEmpty(this.props.personalDetails));
Because we want to freeze it before mounting the report list, we need to move up the onLayout
to get the screen height to ScreenWrapper
. It will result in a bigger height.
<ScreenWrapper
style={screenWrapperStyle}
+ onLayout={(event) => {
+ const skeletonViewContainerHeight = event.nativeEvent.layout.height;
+
+ // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
+ // takes up so we can set the skeleton view container height.
+ if (skeletonViewContainerHeight === 0) {
+ return;
+ }
+ reportScreenHeight = skeletonViewContainerHeight;
+ this.setState({skeletonViewContainerHeight});
+ }}
>
<Freeze
freeze={freeze}
placeholder={(
<>
<ReportHeaderSkeletonView animate={animatePlaceholder} />
<ReportActionsSkeletonView animate={animatePlaceholder} containerHeight={this.state.skeletonViewContainerHeight} />
</>
)}
>
<FullPageNotFoundView
shouldShow={!this.props.report.reportID}
subtitleKey="notFound.noAccess"
shouldShowCloseButton={false}
shouldShowBackButton={this.props.isSmallScreenWidth}
onBackButtonPress={() => {
Navigation.navigate(ROUTES.HOME);
}}
>
<OfflineWithFeedback
pendingAction={addWorkspaceRoomOrChatPendingAction}
errors={addWorkspaceRoomOrChatErrors}
shouldShowErrorMessages={false}
>
<HeaderView
reportID={reportID}
onNavigationMenuButtonClicked={() => Navigation.navigate(ROUTES.HOME)}
personalDetails={this.props.personalDetails}
report={this.props.report}
policies={this.props.policies}
/>
</OfflineWithFeedback>
{this.props.accountManagerReportID && ReportUtils.isConciergeChatReport(this.props.report) && this.state.isBannerVisible && (
<Banner
containerStyles={[styles.mh4, styles.mt4, styles.p4, styles.bgDark]}
textStyles={[styles.colorReversed]}
text={this.props.translate('reportActionsView.chatWithAccountManager')}
onClose={this.dismissBanner}
onPress={this.chatWithAccountManager}
shouldShowCloseButton
/>
)}
<View
nativeID={CONST.REPORT.DROP_NATIVE_ID}
style={[styles.flex1, styles.justifyContentEnd, styles.overflowHidden]}
- onLayout={(event) => {
- const skeletonViewContainerHeight = event.nativeEvent.layout.height;
-
- // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
- // takes up so we can set the skeleton view container height.
- if (skeletonViewContainerHeight === 0) {
- return;
- }
- reportActionsListViewHeight = skeletonViewContainerHeight;
- this.setState({skeletonViewContainerHeight});
- }}
>
{(this.isReportReadyForDisplay() && !isLoadingInitialReportActions) && (
<>
<ReportActionsView
reportActions={this.props.reportActions}
report={this.props.report}
session={this.props.session}
isComposerFullSize={this.props.isComposerFullSize}
isDrawerOpen={this.props.isDrawerOpen}
parentViewHeight={this.state.skeletonViewContainerHeight}
/>
<ReportFooter
errors={addWorkspaceRoomOrChatErrors}
pendingAction={addWorkspaceRoomOrChatPendingAction}
isOffline={this.props.network.isOffline}
reportActions={this.props.reportActions}
report={this.props.report}
isComposerFullSize={this.props.isComposerFullSize}
onSubmitComment={this.onSubmitComment}
/>
</>
)}
{/* Note: The report should be allowed to mount even if the initial report actions are not loaded. If we prevent rendering the report while they are loading then
we'll unnecessarily unmount the ReportActionsView which will clear the new marker lines initial state. */}
{(!this.isReportReadyForDisplay() || isLoadingInitialReportActions) && (
<ReportActionsSkeletonView
containerHeight={this.state.skeletonViewContainerHeight}
/>
)}
<PortalHost name={CONST.REPORT.DROP_HOST_NAME} />
</View>
</FullPageNotFoundView>
</Freeze>
</ScreenWrapper>
Last thing, we need to receive the onLayout
props at ScreenWrapper
.
return (
<View
+ {...this.props}
style={[
...this.props.style,
styles.flex1,
paddingStyle,
]}
>
....
And yes, I think the animation does not work correctly on the web. Let me know if I miss something.
Result
I don't think we want to use the darker color for the elements in the LHN loading state, maybe we can use the border color instead.
Got it. So, the background color will use the border color, what about the foreground color? Because the foreground color also uses border color.
@shawnborton
The LHN background color should not be the border color, it should be the appHighlightBG color (I forget what it's called in the code exactly, but it's darker than the border color)
Both highlightBG and sidebar/LHN background color use greenHighlightBackground color (#002E22) which is why I'm trying to use a different background and foreground color for the LHN skeleton
We should not be changing the background color of the LHN.
On Tue, Dec 20, 2022 at 9:31 AM bernhardoj @.***> wrote:
Both highlightBG and sidebar/LHN background color use greenHighlightBackground color (#002E22) which is why I'm trying to use a different background and foreground color for the LHN skeleton
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/12698#issuecomment-1359453602, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARWH5QDVHRBRQ5CFSDLISLWOG7N3ANCNFSM6AAAAAAR6PPPDU . You are receiving this because you were mentioned.Message ID: @.***>
I think I should've use of
instead of for
😅. I mean the color of the LHN skeleton, not the LHN.
The skeleton needs 2 colors, background and foreground. The background color used in report screen skeleton is the same as the background color of the LHN, so the LHN skeleton is not noticeable if we use that color.
This is the color used by report screen skeleton
<SkeletonViewContentLoader
key={`skeletonViewLines${index}`}
animate={props.animate}
height={skeletonHeight}
backgroundColor={themeColors.highlightColor}
foregroundColor={themeColors.border}
style={styles.mr5}
So, basically we need a different color for the LHN skeleton.
Ah, got it. If that's the case, we could either try:
greenIcons
and greenBorders
for the two colorsgreenBorders
for one color, and add in a new color that isn't as dark as the LHN background but isn't as bright as the greenBorders
colorThere are some options to disable header buttons (search, profile) on LHN while content is loading. Because those buttons are meaningless before personal details/reports are loaded
@shawnborton @roryabraham what's your preferred option?
I recommend 3. because Chats
title, search, avatar icons are not dynamic and they are clear to display. Only the concern is default avatar.
I tend to agree with you, 3 makes sense to me.
Great. Thanks for the suggestion. I will be back with the new update.
Following my previous proposal here, here is the new update.
Because the new update is for the sidebar, we can ignore the sidebar changes from my previous proposal.
Same skeleton view with different color. I tried 2 colors combination. 1.
backgroundColor={themeColors.icon}
foregroundColor={themeColors.border}
2.
backgroundColor={themeColors.border}
foregroundColor={themeColors.appBG}
The result is at the bottom, let me know which one is better.
To disable the search and settings button, we need to check if personal details or reports is empty. Also, I move the LHN skeleton previously at LHNOptionsList empty component to the Freeze placeholder at SidebarLinks.
+// Keep a reference to the screen height so we can use it when a new SidebarLinks component mounts
+let sidebarHeight = 0;
class SidebarLinks extends React.Component {
constructor(props) {
super(props);
this.showSearchPage = this.showSearchPage.bind(this);
this.showSettingsPage = this.showSettingsPage.bind(this);
this.showReportPage = this.showReportPage.bind(this);
+ this.state = {
+ skeletonViewContainerHeight: sidebarHeight,
+ }
}
...
render() {
- // Wait until the personalDetails are actually loaded before displaying the LHN
- if (_.isEmpty(this.props.personalDetails)) {
- return null;
- }
+ const isLoading = _.isEmpty(this.props.personalDetails) || _.isEmpty(this.props.chatReports);
+ const freeze = isLoading || (this.props.isSmallScreenWidth && !this.props.isDrawerOpen && this.isSidebarLoaded);
const optionListItems = SidebarUtils.getOrderedReportIDs(this.props.reportIDFromRoute);
return (
<View
accessibilityElementsHidden={this.props.isSmallScreenWidth && !this.props.isDrawerOpen}
accessibilityLabel="List of chats"
style={[styles.flex1, styles.h100]}
+ onLayout={(event) => {
+ const skeletonViewContainerHeight = event.nativeEvent.layout.height;
+
+ // The height can be 0 if the component unmounts - we are not interested in this value and want to know how much space it
+ // takes up so we can set the skeleton view container height.
+ if (skeletonViewContainerHeight === 0) {
+ return;
+ }
+ sidebarHeight = skeletonViewContainerHeight;
+ this.setState({skeletonViewContainerHeight});
+ }}
>
<View
style={[
styles.flexRow,
styles.ph5,
styles.pv3,
styles.justifyContentBetween,
styles.alignItemsCenter,
]}
nativeID="drag-area"
>
<Header
textSize="large"
title={this.props.translate('sidebarScreen.headerChat')}
accessibilityLabel={this.props.translate('sidebarScreen.headerChat')}
accessibilityRole="text"
shouldShowEnvironmentBadge
/>
<Tooltip text={this.props.translate('common.search')}>
<TouchableOpacity
accessibilityLabel={this.props.translate('sidebarScreen.buttonSearch')}
accessibilityRole="button"
style={[styles.flexRow, styles.ph5]}
onPress={this.showSearchPage}
+ disabled={isLoading}
>
<Icon src={Expensicons.MagnifyingGlass} />
</TouchableOpacity>
</Tooltip>
<TouchableOpacity
accessibilityLabel={this.props.translate('sidebarScreen.buttonMySettings')}
accessibilityRole="button"
onPress={this.showSettingsPage}
+ disabled={isLoading}
>
<AvatarWithIndicator
source={this.props.currentUserPersonalDetails.avatar}
tooltipText={this.props.translate('common.settings')}
/>
</TouchableOpacity>
</View>
- <Freeze freeze={this.props.isSmallScreenWidth && !this.props.isDrawerOpen && this.isSidebarLoaded}>
+ <Freeze
+ freeze={freeze}
+ placeholder={<LHNSkeletonView containerHeight={this.state.skeletonViewContainerHeight} />}
+ >
<LHNOptionsList
contentContainerStyles={[
styles.sidebarListContainer,
{paddingBottom: StyleUtils.getSafeAreaMargins(this.props.insets).marginBottom},
]}
data={optionListItems}
focusedIndex={_.findIndex(optionListItems, (
option => option.toString() === this.props.reportIDFromRoute
))}
onSelectRow={this.showReportPage}
shouldDisableFocusOptions={this.props.isSmallScreenWidth}
optionMode={this.props.priorityMode === CONST.PRIORITY_MODE.GSD ? 'compact' : 'default'}
onLayout={() => {
App.setSidebarLoaded();
this.isSidebarLoaded = true;
}}
/>
</Freeze>
</View>
);
}
}
Result Sorry, it's lagging. 1.
2.
And because we move the skeleton to the Freeze component, now when we partially slide the report screen, it will show the skeleton, which previously just show a blank page.
Do you mind pulling main to get the latest colors? We made some small adjustments to the dark mode styles.
In terms of colors though, I am thinking that we should provide a new color that is a bit lighter than the border color you are using but not as light as the icon color. Could you try with #2B5548 as the lighter color, so we get this:
Sure.
Here is the result: background color: #2B5548 (any idea what we should name this color? themeColors.? => colors.?) foreground color: themeColors.border => colors.greenBorders (#1A3D32)
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
sign in to the app
Expected Result:
the skeleton UI appears and then is replaced by the reports / normal LHN
Actual Result:
you’ll see a blank screen, and on my iPhone 14 Pro the FAB appears as barely visible in the top right of the screen
Workaround:
unknown
Platform:
Where is this issue occurring?
Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:
Feature request was posted in slack couple weeks ago. Was waiting for reproduction. Then I captured screenshot with slow 3G
Expensify/Expensify Issue URL: Issue reported by: @roryabraham Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666890950340499
View all open jobs on GitHub