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 2021-12-06] [HOLD for payment 2021-11-25] Update "Be the first person to comment!" screen when you start a new DM #4678

Closed
mallenexpensify opened this issue Aug 16, 2021 · 118 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@mallenexpensify
Copy link
Contributor

mallenexpensify commented Aug 16, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

Sent DM message to someone I hadn't messaged before

Expected Result:

The blank screen should have avatars, a message for "This is the beginning of your chat history with XXXX (pronouns), as well as showing the timezone above the message input when messaging someone 1:1.

Actual Result:

Only see "Be the first person to comment!"

Solution

Let's update this state/area to include the users icon, name, preferred pronouns and the time in their time zone (only if 1:1 DM).
image

Design files are here in Figma

Platform:

Where is this issue occurring?

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

Expensify/Expensify Issue URL: link

View all open jobs on Upwork

Upwork job: https://www.upwork.com/jobs/~0159561fdd76ccd036

@mallenexpensify mallenexpensify added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 16, 2021
@MelvinBot
Copy link

Triggered auto assignment to @michaelhaxhiu (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 Aug 16, 2021
@Santhosh-Sellavel
Copy link
Collaborator

@mallenexpensify

What's it gonna be for the new group chat?

Screenshot 2021-08-17 at 12 37 42 AM

@mallenexpensify
Copy link
Contributor Author

Great point @Santhosh-Sellavel ! I addressed your question in an old internal issue we had been working on, I instructed folks to post comments/feedback to this issue. We have two choices

  1. Address group chats in this GH and figure out what we want before posting the job.
  2. Only focus on the single DM usecase for now and address groups later.

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 16, 2021

Is this going to be external? @mallenexpensify
If it's I'm interest then.

An overview of implementation details,

  • Will create a new component to show the required details.
  • Will use component instead of current textview, which shows 'be first to comment' in ReportActionsView.js
  • And add relevant styles or reuse available styles.
  • Also change placeholder text in chat input.

@mallenexpensify
Copy link
Contributor Author

It will very likely be external @Santhosh-Sellavel , @michaelhaxhiu needs to triage the issue first, part of that is figuring out what we want to do about group messages. My hunch is it's best to fix this issue specifically for the 1:1 DM usecase now then create a new issue in the future to address both group messages and possibly rooms.

@michaelhaxhiu, this shouldn't require a designer, an engineer should be able to take care of everything

@rosegrech
Copy link

Great question about the group. I want to say lets work on how the 1:1 looks like and then probably handle the group in a new GH to keep things neat and tidy. I'd love to say whatever we use for 1:1 will work for the group but ard to say that as of now :)

@shawnborton
Copy link
Contributor

@megankelso @michelle-thompson I vaguely recall that we explored what this screen would look like for various use cases... actually I think it was @michelle-thompson that did it for Concierge Travel.

So basically we could do something like this?
image

And maybe before we have those features ready to go, we could just use a generic message for the group situation like so:
image

And then in terms of scalability, looks like Michelle had mocked up something like this for when a group has lots of participants:
image

@michelle-thompson
Copy link
Contributor

Cool, happy to move forward with the generic messaging you've mocked up above!

@shawnborton
Copy link
Contributor

Sweet, so I think maybe we want to make the avatars in my mockup a bit smaller like your original mocks (the one at the bottom) for scalability purposes. Otherwise, then I think we just need to decide if we're going to add Group chats to the exported Upwork issue?

@michelle-thompson
Copy link
Contributor

Ahh, you mean so the size of the avatar never has to change? And I figure might as well add group chats to this issue.

@shawnborton
Copy link
Contributor

Yeah, I think I like the size you had here:
image

...better than the larger size I used. So that gives us something like this:
image

@michelle-thompson
Copy link
Contributor

Agreed, I think that looks good too

@megankelso
Copy link

I agree, this looks good! I do wonder if we should make this a bit more consistent. What comes to mind for me is using the existing UI pattern to show local time (above the chat input) and including the same "this is the beginning for your chat" message, but perhaps adding the pronouns. Curious for thoughts?

Screen Shot 2021-08-17 at 11 19 20 AM

@michelle-thompson
Copy link
Contributor

michelle-thompson commented Aug 17, 2021

I'm into it! Though curious what this would look like if you were to DM someone in the same time zone?

@megankelso
Copy link

I'm suggesting it still show the time for the first chat, regardless of what time zone. Curious if you had something else in mind?

@michelle-thompson
Copy link
Contributor

Ahhh, I see. I think that would be fine too, I'm keen with either direction!

@shawnborton
Copy link
Contributor

Oh, I really like that idea about the timezone. Actually taking this a step further, perhaps the name above the message for the 1:1 case is redundant as well? So that leaves us with this:

image

@megankelso
Copy link

yes, agreed! we can drop the name. I like the addition of pronouns to the group but I'm not sure how scaleable it is for larger group chats. For example, if we added them to the next screen:

Screen Shot 2021-08-18 at 11 48 15 AM

Curious for thoughts. would it be weird to make a rule that pronouns are included only for group chats of 3 people and under or so?

@michelle-thompson
Copy link
Contributor

I think you can have up to eight people in DMs, so it's possible to include pronouns for everyone! It looks a little busy, but I don't particularly mind that since this screen is temporary.

image

@shawnborton
Copy link
Contributor

I agree, I think it will be okay. It's probably not very likely that all 8 members of the group will have their pronouns set anyways.

@MelvinBot
Copy link

@michaelhaxhiu Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@mallenexpensify
Copy link
Contributor Author

@shawnborton @michelle-thompson will let you @michaelhaxhiu know when he should review it for triage? Also.. huge, double plus, bonus points if you can update the OP with the deliverables for the contributors.

@shawnborton
Copy link
Contributor

Cool, I updated the original comment with the new screenshots, I think this is ready for @michaelhaxhiu

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 23, 2021
@jboniface
Copy link

not sure where this one is at

@MelvinBot MelvinBot removed the Overdue label Nov 9, 2021
@roryabraham
Copy link
Contributor

I left some review comments about a week ago but the PR hasn't been updated. I know there have been lots of delays on our side, but @MirFahad58 let's get this issue over the finish line!

@MirFahad58
Copy link
Contributor

@roryabraham i will update asap.

@MirFahad58
Copy link
Contributor

@michaelhaxhiu @roryabraham @shawnborton
I think PR is ready to merge all checks have passed.
Screenshot 2021-11-17 at 1 40 51 PM

@MirFahad58
Copy link
Contributor

and I left some messages on U.W need the response

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Nov 17, 2021

and I left some messages on U.W need the response

@MirFahad58 can you make sure to send any messages concerning your progress in GH? We have more visibility here and there are more eyes to weigh in on your inquiries.

@MirFahad58
Copy link
Contributor

@michaelhaxhiu I requested a bonus milestone for n6-hold on U.W and want to submit the work for the current milestone as I have to match my expenses for this month. Thanks

@roryabraham
Copy link
Contributor

PR merged 🎉

@MirFahad58
Copy link
Contributor

MirFahad58 commented Nov 18, 2021

@michaelhaxhiu @mallenexpensify as PR is merged now. I need response for above comment. Please reply

@michaelhaxhiu
Copy link
Contributor

@MirFahad58

We'll pay the remaining amount due after 7 days of no regressions.

Let's do a quick buddy check on the outstanding payment that's owed for your work, as I'd like to make sure it's fair!

  1. $500 for completing the job (The job was initially $250, but we increased that to $500 due to increase scope - context)
  2. $250 bonus for the n6 hold (Already paid via Milestone in Upwork)

image

@MirFahad58
Copy link
Contributor

MirFahad58 commented Nov 18, 2021

@michaelhaxhiu let me correct, the 250 that was paid, was not of n6-hold bonus it was half of $500 from jobs. Thanks
that's why i added a milestone for n6-hold

Screenshot 2021-11-18 at 11 21 01 PM

@michaelhaxhiu
Copy link
Contributor

Ok so it sounds like you are saying:

  1. The $250 that was already paid was for your initial work ✅
  2. The $250 n6 hold bonus hasn't been sent yet, and you are requesting that be sent today
  3. Finally, we owe $250 for the increased scope of work, which will be paid in 6 days.

Is that right?

@MirFahad58
Copy link
Contributor

right

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Nov 18, 2021

Ok got it. I think that is fine, I'll issue the $250 payment now.

Then, you'll receive the remaining $250 on November 25.

Note: our milestones aren't named correctly, but thats ok because they are in $250 incraments.

@MirFahad58
Copy link
Contributor

yup thanks

@michaelhaxhiu michaelhaxhiu changed the title Update "Be the first person to comment!" screen when you start a new DM [HOLD for payment 2021-11-25] Update "Be the first person to comment!" screen when you start a new DM Nov 18, 2021
@mallenexpensify
Copy link
Contributor Author

Paid final $250 to @MirFahad58 , thanks for the help and patience.

@botify botify added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Nov 29, 2021
@botify botify changed the title [HOLD for payment 2021-11-25] Update "Be the first person to comment!" screen when you start a new DM [HOLD for payment 2021-12-06] [HOLD for payment 2021-11-25] Update "Be the first person to comment!" screen when you start a new DM Nov 29, 2021
@botify
Copy link

botify commented Nov 29, 2021

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.1.16-10 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 2021-12-06. 🎊

@melvin-bot
Copy link

melvin-bot bot commented Aug 25, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot
Copy link

melvin-bot bot commented Sep 2, 2022

⚠️ Looks like this issue was linked to a possible regression on PRODUCTION here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a production regression has occurred a Root Cause Analysis is required. Please follow the instructions here.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

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 External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Development

No branches or pull requests