Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2022-08-11 - Reminder to add bonus] Add skeleton UI to chats #7081

Closed
mvtglobally opened this issue Jan 7, 2022 · 142 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

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:

  1. Open app
  2. Make sure you are on a slow connection (e.g. 3G)
  3. Open a new chat

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?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

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
Image from iOS (9)

Image from iOS (10)

Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1641514266065100

View all open jobs on GitHub

@mvtglobally mvtglobally added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Jan 7, 2022
@MelvinBot
Copy link

Triggered auto assignment to @johncschuster (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jan 7, 2022
@shawnborton
Copy link
Contributor

Following this one, I'd love to mock something up for this.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Jan 7, 2022

Are we planning to add a shimmering effect (as shown in the 2nd screenshot) or some placeholders here?

@johncschuster
Copy link
Contributor

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.

@puneetlath
Copy link
Contributor

@johncschuster it seems like we have agreement in the slack thread to go forward with it.

@johncschuster
Copy link
Contributor

Sweet! In that case, I shall triage the thing. Thanks, @puneetlath!

@MelvinBot
Copy link

Triggered auto assignment to @madmax330 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@johncschuster johncschuster removed their assignment Jan 7, 2022
@madmax330
Copy link
Contributor

@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?

@MelvinBot MelvinBot removed the Overdue label Jan 19, 2022
@puneetlath
Copy link
Contributor

@shawnborton suggested going with a ghost UI like this in the slack thread.

image

@shawnborton do you feel good about going forward with that?

@shawnborton
Copy link
Contributor

Yup! And then I like the idea of adding some kind of slight fading/pulsing effect to it.

@madmax330
Copy link
Contributor

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)

@shawnborton
Copy link
Contributor

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.

@mananjadhav
Copy link
Collaborator

mananjadhav commented Jan 20, 2022

I have one or two options to implement, waiting for this to go "External" before applying.

@madmax330
Copy link
Contributor

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

@madmax330 madmax330 added the External Added to denote the issue can be worked on by a contributor label Jan 20, 2022
@MelvinBot
Copy link

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@stephanieelliott
Copy link
Contributor

Shared to Upwork: https://www.upwork.com/jobs/~01a3b83272750b939a

@stephanieelliott stephanieelliott changed the title FEATURE REQUEST: Add some background when chat is empty on slot connection Add some background when chat is empty on slot connection Jan 20, 2022
@botify botify removed the Daily KSv2 label Jan 20, 2022
@stephanieelliott stephanieelliott added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jul 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Jul 13, 2022

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 13, 2022
@stephanieelliott
Copy link
Contributor

Hey @MitchExpensify, reassigning this as I am going OOO. To save you some scrolling, the PR for this one is #8042, most of the action is happening there now. Thank you!

@parasharrajat
Copy link
Member

PR is almost done.

@melvin-bot
Copy link

melvin-bot bot commented Jul 26, 2022

@mananjadhav, @parasharrajat, @MitchExpensify, @Luke9389 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Luke9389
Copy link
Contributor

Hmm, we shouldn't be seeing the Overdue message with the Reviewing label applied 🤔

@marcaaron
Copy link
Contributor

also no Overdue label applied 🤔

@melvin-bot
Copy link

melvin-bot bot commented Aug 3, 2022

@mananjadhav, @parasharrajat, @MitchExpensify, @Luke9389 Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Aug 4, 2022
@melvin-bot melvin-bot bot changed the title Add skeleton UI to chats [HOLD for payment 2022-08-11] Add skeleton UI to chats Aug 4, 2022
@melvin-bot
Copy link

melvin-bot bot commented Aug 4, 2022

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.87-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2022-08-11. 🎊

@mananjadhav
Copy link
Collaborator

@Luke9389 @marcaaron @parasharrajat I was wondering considering the effort put in the issue, is it possible to increase the budget on this?

@marcaaron
Copy link
Contributor

Hmm I'm not sure. I see this:

If you think your compensation should be increased for a specific job, you can request a reevaluation by commenting in the Github issue where the Upwork job was posted.

In the doc, but not really clear to me what the process for re-evaluation. @mananjadhav can you maybe give us a brief summary with something like estimated total number of hours you spent working on this, why you feel the compensation should be increased, etc?

@mananjadhav
Copy link
Collaborator

Sure @marcaaron.

I don't have a specific total number of hours honestly. But I can explain the reasons behind the request.

  1. Earlier we agreed to add the Skeleton UI only to the first load of the chat and not load more. So we ended up discussing and then agreeing to replace the logic with load more as well

  2. I didn't factor any animation with the component, and had only added pulse animation to the chat load more. Getting the right animation required some back and forth and agreement across the team, which required some effort.

  3. (this could partially be put on me as the delay was due to my unverified commit), but by the time we were ready to merge the PR, we changed the logic for the Onyx Keys handling for loading the reports and had to redo those changes as per the new format.

I hope this helps.

cc - @parasharrajat

@marcaaron
Copy link
Contributor

marcaaron commented Aug 10, 2022

I don't have a specific total number of hours honestly. But I can explain the reasons behind the request.

What areas would you say that spent "most of your time" working on? Can you break it down into chunks? Can you express each of these additional areas as a percentage of the total time you spent working?

@mananjadhav
Copy link
Collaborator

@marcaaron I went through some logs and here's an update if this helps.

  1. Load More handling - Updates on the solution, code and work ~2-3 hours
  2. Animation - This overall took maximum time, from two aspects. One was the actual implementation, second some trial and error on my end to figure our if each item can be put inline with the animation. ~6-8 hours.
  3. Rework based on new Keys in Onyx 1 hour.

@marcaaron
Copy link
Contributor

@mananjadhav thanks, just one more question, can you tell us what you feel is a fair compensation for this issue so we can review?

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 10, 2022

what you feel is a fair compensation for this issue so we can review?

Thanks for the response @marcaaron. I feel it should be 1000$

@marcaaron
Copy link
Contributor

@Luke9389 @MitchExpensify seems fair to me based on the scope of the job and seems like maybe it was priced a bit low from the beginning.

@MitchExpensify
Copy link
Contributor

Cool, I'll add a $750 bonus when paying tomorrow.

@MitchExpensify MitchExpensify changed the title [HOLD for payment 2022-08-11] Add skeleton UI to chats [HOLD for payment 2022-08-11 - Reminder to add bonus] Add skeleton UI to chats Aug 10, 2022
@Luke9389
Copy link
Contributor

I agree with that new total 👍

@MitchExpensify
Copy link
Contributor

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Engineering External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests