Closed mvtglobally closed 2 years ago
Triggered auto assignment to @johncschuster (AutoAssignerTriage
), see https://stackoverflow.com/c/expensify/questions/4749 for more details.
Following this one, I'd love to mock something up for this.
Are we planning to add a shimmering effect (as shown in the 2nd screenshot) or some placeholders here?
Is this feature request still in the planning phase? If so, I'm not sure it would be appropriate to triage this to engineering right now.
@johncschuster it seems like we have agreement in the slack thread to go forward with it.
Sweet! In that case, I shall triage the thing. Thanks, @puneetlath!
Triggered auto assignment to @madmax330 (Engineering
), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
@puneetlath have we decided on what the background should be? I.e do we already have the resources for it or does design need to come up with something first?
@shawnborton suggested going with a ghost UI like this in the slack thread.
@shawnborton do you feel good about going forward with that?
Yup! And then I like the idea of adding some kind of slight fading/pulsing effect to it.
Cool do we already have that asset or does that need to be created as part of this issue? (just trying to determine if it can be marked as external or not)
I suppose they are just shapes, so perhaps we don't need assets and we can just create them using containers and styles? Otherwise we can provide all of these as .svgs. Maybe it just depends on the proposals we get - I suppose there are multiple ways to implement this (either by creating containers or using svgs) and I'm curious which way you think is best.
I have one or two options to implement, waiting for this to go "External" before applying.
I'm curious which way you think is best
I'm honestly not sure. But my guess would be svg, that way we're sure it'll render the same on all platforms. Not sure if making it with containers will have issue with different platforms. That's just a shot in the dark though. I guess we'll mark it as external and see what the proposals are
Triggered auto assignment to @stephanieelliott (External
), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Shared to Upwork: https://www.upwork.com/jobs/~01a3b83272750b939a
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported
)
Triggered auto assignment to @Luke9389 (Exported
), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
Yeah, I agree with Shawn. It can be done in many ways. Also, if we need to show the moving gradient effect over them then that needs to be animated. I am sure contributors will propose that as well. But can we please attach the image to issue body.
Proposal:
I will use an Svg image for the skeleton then animate a gradient moving over the skeleton with react-native-reanimated
.
The skeleton (content-lazy-loading.svg) and the gradient (lazy-loader-gradient.svg) will be placed in :
App/assets/images/
then the component housing the logic will be placed in :
src/components/LazyLoader.js
The LazyLoader
component will be rendered here:
https://github.com/Expensify/App/blob/0bce09177d2aed8c5e377bc758f5241dd22886eb/src/pages/home/ReportScreen.js#L177
replacing <FullScreenLoadingIndicator visible={this.shouldShowLoader()} />
with
this.shouldShowLoader() && <LazyLoader />
Are we guys looking to create a small component with SVGs? or a reusable component so that it can be like a library. I've got two ways to solve this.
Common Code in both approaches:
In ReportScreen
, we place FullScreenLoadingIndicator
with GhostReportScreen
component described later.
1. Single-Use Component with SVGs
GhostReportScreen
with a basic layout of Avatar
and View
with Images loaded with SVG
.2. Reusable lib without images
We create a GhostUI
component that can be reused for any loader component. A sample (crude UI) looks like this:
https://user-images.githubusercontent.com/3069065/150652839-6f494572-2813-48f5-9ea2-3e7d7393b01c.mov
Its code flow then looks like:
Initialize the animation instance with and two functions startAnimation()
and stopAnimation()
this.pulsingAnimation = new Animated.Value(this.props.animationConfig.startX);
this.gradientOpacity = [0.05, 0.15, 0.25, 0.3, 0.35, 0.3, 0.25, 0.15, 0.05];
startAnimation
and stopAnimation
will be called in componentDidMount
, componentDidUpdate
, etc. based on a prop isAnimating
stopAnimation
will just call this.pulsingAnimation.stop()
where as startAnimation
, will call Animated.loop
,
startAnimation() {
const animationConfig = this.props.animationConfig;
Animated.loop(
Animated.timing(this.pulsingAnimation, {
toValue: animationConfig.toValue || Dimensions.get('window').width + 100,
duration: 2000,
}),
).start();
}
render function for the GhostUI
<View>
<Animated.View
style={{
transform: [
{
translateX: this.pulsingAnimation,
},
],
top: 0,
bottom: 0,
zIndex: 100,
position: 'absolute',
flexDirection: 'row',
}}
>
{_.map(this.gradientOpacity, (opacity, i) => (
<View
key={i}
style={{
transform: [
{
skewX: '-30deg',
},
],
width: 20,
height: '100%',
backgroundColor: this.props.animationConfig.animationColor,
opacity,
}}
/>
))}
</Animated.View>
{this.props.children}
</View>
GhostReportScreen
, which will have the Avatar
, View
, etc. wrapped within <GhostComponent animationConfig={animationConfig} isAnimating>
I wasn't sure on best way to shorten the proposal, I hope it isn't tedious to review (a lot is related to Animated
).
If we go ahead with point 1, then I am fine with a small budget, but if we take 2 I feel the budget should be increased as its going to be dynamic resuablle component and purely View
, etc based with no additional dependencies.
Thanks for the proposals guys.
Using SVG Loader seems a good solution.
But we are missing one important step. How will you detect that chat is loading?
this.shouldShowLoader
is an illusion to give a sense of loading to match Native platforms. it does not really track the report loading.
But we are missing one important step. How will you detect that chat is loading?
this.shouldShowLoader
is an illusion to give a sense of loading to match Native platforms. it does not really track the report loading.
@parasharrajat How about using this.props.isLoadingReportActions
from withOnyx
?
First we initialize showSVGLoader
to true
this.state = {
isLoading: true,
showSVGLoader:true
};
When this.props.isLoadingReportActions
is false
we set showSVGLoader
to false
componentDidUpdate(prevProps) {
if(!this.props.isLoadingReportActions && this.state.showSVGLoader){
this.setState({showSVGLoader:false})
}
.....
this.state.showSVGLoader && <LazyLoader />
export default withOnyx({
isLoadingReportActions: {
key: ONYXKEYS.IS_LOADING_REPORT_ACTIONS,
},
....
@brianmuks This key will be true many times when users will scroll up to load paginated messages. which will show the loader again and again.
How will you tackle this issue?
@parasharrajat That is a good concern. kindly note that in the componentDidUpdate
I am setting the local state (showSVGLoader)
to false
only once and it never goes back to true
again. Meaning the lazyLoader
won't be re-render when new users scroll up to load new messages.
componentDidUpdate(prevProps) {
// I expect this to evaluate to true only once since I am never calling this.setState({showSVGLoader:true}) anywhere
if(!this.props.isLoadingReportActions && this.state.showSVGLoader){
this.setState({showSVGLoader:false})
}
Oh, yeah. My bad completely overlooked it. But still, can we use ONYXKEYS.IS_LOADING_REPORT_DATA
instead? I think when this is true all the initial data is loaded. Will it not work?
I can go with that.
So you are saying that it will work. Have you tested it?
Yes I have. It works
Ok, Sounds good. I didn't get a good sense of how you will implement the lazy loader. If you could explain briefly in technical words, that would be great.
@parasharrajat does this address your question or you need something more detailed. ?
Proposal: I will use an Svg image for the skeleton then animate a gradient moving over the skeleton with
react-native-reanimated
. The skeleton (content-lazy-loading.svg) and the gradient (lazy-loader-gradient.svg) will be placed in :App/assets/images/
then the component housing the logic will be placed in :
src/components/LazyLoader.js
The
LazyLoader
component will be rendered here:replacing
<FullScreenLoadingIndicator visible={this.shouldShowLoader()} />
withthis.shouldShowLoader() && <LazyLoader />
Off-course keeping in mind the latest suggestions
I will use an Svg image for the skeleton then animate a gradient moving over the skeleton with react-native-reanimated. The skeleton (content-lazy-loading.svg) and the gradient (lazy-loader-gradient.svg) will be placed in
I am asking to expand on this part.
I am asking to expand on this part.
So the whole idea is create small component that will render two SVG images. The first SVG image will be static one like the one below:
The second SVG image will be a gradient images that will move back and forth over the first SVG image using react-native-reanimated
. The animation will be stopped when the component is unmounted:
Ok. Sounds good.
I like @brianmuks 's proposal. @mananjadhav has a great proposal too but @brianmuks came up first with the idea and @mananjadhav I see that you are already assigned to a couple of issues so this could be a good start for @brianmuks.
cc: @Luke9389
:ribbon: :eyes: :ribbon: C+ reviewed
Is there a mockup for what this will look like on web/desktop? I wonder if we should have a different static svg for the wider screen sizes. @shawnborton did you happen to make a mockup for larger screens?
Actually wider screens is a great point. I think that strengthens the argument to not use an .svg for the actual placeholder shape, this way we can use a container for the fake text that can stretch as wide as we need it.
I think that strengthens the argument to not use an .svg for the actual placeholder shape
@shawnborton @Luke9389 @parasharrajat Wrt to the comment, do you want to explore solution 2 in this comment. https://github.com/Expensify/App/issues/7081#issuecomment-1019341453
The (not so good-looking) video I've attached with the proposal is pure View
and Text
to load the view.
That's what I am thinking, yeah.
BUt SVG can expand as much as we want. They fill in the available space.
Fair point, but I suppose for that to work, we'd need individual svgs for each element of the placeholder seeing as we'd only stretch some and not all. That's totally fine with me, but at that point, why would that be better than just drawing shapes as views?
Are we okay to explore adding a dependency? https://www.npmjs.com/package/react-content-loader Makes it reusable for future use cases and very small size footprint.
No extra processing. SVG runs on the UI layer.
But I would say, lets do whatever is easier.
After reading the code for this package, https://www.npmjs.com/package/react-content-loader, I would be OK with using it. It uses another package called 'react-native-svg' to handle svgs in the native environment, and uses an <svg>
tag for web.
I'm going to open this up for a few more opinions before moving forward. See you all on slack!
Here's the slack convo I mentioned above: https://expensify.slack.com/archives/C01GTK53T8Q/p1643743784049659
We're OK with using react-content-loader
👍
@parasharrajat, how would you like to do this? react-content-loader
was @mananjadhav's idea. @mananjadhav are you still busy with other issues?
I think we still want a full proposal either way, that talks about using react-content-loader
. Do you agree @parasharrajat ?
Yeah, package looks good to me it uses SVG which is cool.
Let's give this opportunity to @mananjadhav . @mananjadhav could you please post a new proposal explaining bits and pieces as now it's clear on what should be done?
I'll send in a proposal in a few hours.
Revised Proposal
react-content-loader
as a dependencyChatGhostUI
import React from 'react';
import ContentLoader from 'react-content-loader';
import {Dimensions} from 'react-native';
const ChatList = (props) => {
const windowWidth = Dimensions.get('window').width;
const windowHeight = Dimensions.get('window').height;
return (
<ContentLoader viewBox={`0 0 ${windowWidth} ${windowHeight}`} height="100%" width="100%" {...props}>
<circle cx="70.2" cy="73.2" r="41.3" />
<rect x="129.9" y="29.5" width="80%" height="17" />
<rect x="129.9" y="64.7" width="296" height="17" />
<rect x="129.9" y="97.8" width="253.5" height="17" />
<rect x="129.9" y="132.3" width="212.5" height="17" />
<circle cx="70.7" cy="243.5" r="41.3" />
<rect x="130.4" y="199.9" width="125.5" height="17" />
<rect x="130.4" y="235" width="296" height="17" />
<rect x="130.4" y="268.2" width="253.5" height="17" />
<rect x="130.4" y="302.6" width="212.5" height="17" />
<circle cx="70.7" cy="412.7" r="41.3" />
<rect x="130.4" y="369" width="125.5" height="17" />
<rect x="130.4" y="404.2" width="296" height="17" />
<rect x="130.4" y="437.3" width="253.5" height="17" />
<rect x="130.4" y="471.8" width="212.5" height="17" />
</ContentLoader>
);
};
export default ChatList;
index.js
, we'll have to import import ContentLoader, { Rect, Circle } from 'react-content-loader/native'
Here's what it looks like:
https://user-images.githubusercontent.com/3069065/152167749-090fbc67-0706-4c09-9900-b7e1d01ab7a2.mov
Didn't understand this part?
For index.js, we'll have to import import ContentLoader, { Rect, Circle } from 'react-content-loader/native'
How will you make sure that the UI screen is completely filled with loader irrespective of the height of the screen?
How will you make sure that the UI screen is completely filled with loader irrespective of the height of the screen?
Here's the working code.
import React from 'react';
import {Dimensions} from 'react-native';
import ContentLoader from 'react-content-loader';
import _ from 'underscore';
const ChatList = (props) => {
const windowHeight = Dimensions.get('window').height;
const height = 140;
const numberOfRows = Math.floor(windowHeight / height);
const contentItems = Array.from({length: numberOfRows}, (v, i) => i);
return (
<>
{
_.map(contentItems, item => (
<ContentLoader height={height} width="100%" key={item} {...props}>
<circle cx="50" cy="70" r="30" />
<rect x="100" y="64.7" width="30%" height="17" />
<rect x="100" y="29.5" width="85%" height="17" />
<rect x="100" y="97.8" width="60%" height="16" />
</ContentLoader>
))
}
</>
);
};
export default ChatList;
What I'll need help with is, will the size of the placeholder be the same across devices or will change for small or medium devices? Accordingly, I can set sizes for the svgs
Didn't understand this part?
For index.js, we'll have to import import ContentLoader, { Rect, Circle } from 'react-content-loader/native'
I meant index.native.js
. Sorry typo. Instead of using circ
and rect
, we'll import Circle and Rect
for native platforms.
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:
Expected Result:
There should be some identifier that chat is loading
Actual Result:
Below image is the chat empty state when trying to load it on a bad connection. It'd be nice to put something in there while waiting
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.1.26-0 Reproducible in staging?: Y Reproducible in production?: Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL: Issue reported by: @puneetlath Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1641514266065100
View all open jobs on GitHub