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

Update "be the first person to comment" screen #4921

Merged
merged 38 commits into from
Nov 17, 2021

Conversation

MirFahad58
Copy link
Contributor

@MirFahad58 MirFahad58 commented Aug 30, 2021

@michaelhaxhiu @roryabraham

Details

Updated the "Be the first person to comment!" screen to a new look here is the Figma link.
As we discussed in the issue.

Fixed Issues

$ #4678

Tests

Run app on any platform
Login with any account on any platform,

  1. Click on the plus button add a new contact (email/user) or create a new group.
  2. You will see the new and updated screen for "Be the first person to comment"
  3. If you created a new chat with a single user it will show its time zone on top of the input otherwise will not.
  4. Avatar(s) will show for the user in a group or single chat along with a description.
  5. for private groups it will show a custom icon and custom description
    please refer to Figma for more detail.

QA Steps

For example:

  1. Click on the plus button add a new contact (email/user) or create a new group.
  2. You will see the new and updated screen for "Be the first person to comment"
  3. If you created a new chat with a single user it will show its time zone on top of the input otherwise will not.
  4. Avatar(s) will show for the user in a group or single chat along with a description.
  5. for private groups it will show a custom icon and custom description
    previously it was only "be the first to comment" text on the whole screen.

Tested On

  • Web
  • Mobile Web
  • [] Desktop
  • [] iOS
  • Android

Screenshots

Web

Screenshot 2021-08-31 at 12 52 51 AM

Screenshot 2021-08-31 at 12 52 37 AM

Mobile Web

Screenshot 2021-08-31 at 12 53 20 AM

Screenshot 2021-08-31 at 12 53 33 AM

Desktop

iOS

Android

Screenshot_2021-08-31-22-04-59-687_com expensify chat
Screenshot_2021-08-31-22-04-32-002_com expensify chat

@MirFahad58 MirFahad58 requested a review from a team as a code owner August 30, 2021 18:44
@MelvinBot MelvinBot requested review from roryabraham and removed request for a team August 30, 2021 18:44
@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@Santhosh-Sellavel
Copy link
Collaborator

@MirFahad58 Message placeholder should say "Say hello!"

@MirFahad58 MirFahad58 changed the title Fahad be the first person to comment Update "be the first person to comment" screen Aug 30, 2021
@MirFahad58
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@MirFahad58
Copy link
Contributor Author

recheck

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Aug 30, 2021

@MirFahad58 I think it should show Say Hello! for first time Alone.

Check with @shawnborton & others.

Don't forget to update screenshots!

@roryabraham
Copy link
Contributor

@MirFahad58 Looks like this PR has some merge conflicts that need to be addressed.

@MirFahad58
Copy link
Contributor Author

Ok let me resolve that ASAP.

@MirFahad58
Copy link
Contributor Author

updated

@shawnborton
Copy link
Contributor

Looking good so far. Can you please share a screenshot of what this looks like when the group has 8 members in it? Thanks!

@MirFahad58
Copy link
Contributor Author

@shawnborton yes sure but i need a test account with multiple member so please if you have any please provide.

@shawnborton
Copy link
Contributor

Great point - I'm not sure what to recommend for that. @roryabraham any ideas?

@Santhosh-Sellavel
Copy link
Collaborator

@MirFahad58 https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#test-accounts

@MirFahad58
Copy link
Contributor Author

I will update screenshots with multiple users soon.

@MirFahad58
Copy link
Contributor Author

updated

src/styles/styles.js Outdated Show resolved Hide resolved
@MirFahad58
Copy link
Contributor Author

@roryabraham just resolved the conflict please review it.

roryabraham
roryabraham previously approved these changes Nov 17, 2021
@roryabraham
Copy link
Contributor

@MirFahad58 You've got a lint error.

@MirFahad58
Copy link
Contributor Author

@roryabraham code is updated now

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, did we change the room name to bold as well along the way? @MirFahad58

@@ -503,6 +505,9 @@ class ReportActionsView extends React.Component {
}

render() {
const {isDefaultRoom} = ReportUtils;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it hurts. Please call these directly ReportUtils.isDefaultRoom. Otherwise, there is no advantage of import * as ReportUtils from '../../../libs/reportUtils';.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh nope please suggest the code because we are sending the #room in the string as a prop for language.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. You can break them into multiple Text nodes.

<Text>This </Text>
<Text>**room**</Text>
<Text>is awesome</Text>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @parasharrajat, but I'm confused why we would need to break into multiple text nodes... @MirFahad58 why does this not work?

const isDefaultChatRoom = ReportUtils.isDefaultRoom(this.props.report);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should i do like this
beginningOfChatHistoryPrivatePartOne
beginningOfChatHistoryPrivatePartTwo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's fine for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before doing that try to get the two parts of the sentence in Spanish on the Slack.

Copy link
Contributor Author

@MirFahad58 MirFahad58 Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roryabraham which one? separate it into two parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beginningOfChatHistoryPrivatePartOne: Este es el principio de la sala privada
beginningOfChatHistoryPrivatePartOne: room, invite others by @mentioning them.
@parasharrajat @roryabraham what about this?

@@ -503,6 +505,9 @@ class ReportActionsView extends React.Component {
}

render() {
const {isDefaultRoom} = ReportUtils;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @parasharrajat, but I'm confused why we would need to break into multiple text nodes... @MirFahad58 why does this not work?

const isDefaultChatRoom = ReportUtils.isDefaultRoom(this.props.report);

@parasharrajat
Copy link
Member

Lastly, did we change the room name to bold as well along the way?

@roryabraham that comment is for... #4921 (review)

@MirFahad58
Copy link
Contributor Author

I updated the code for the bold text of the room name and other requested changes.
Screenshot 2021-11-18 at 1 24 58 AM
Screenshot 2021-11-18 at 1 27 03 AM

@MirFahad58
Copy link
Contributor Author

I accidentally closed the PR but reopened it. will there be any issue with that?

@roryabraham roryabraham merged commit 3b5605d into Expensify:main Nov 17, 2021
@OSBotify
Copy link
Contributor

@MirFahad58, Great job getting your first Expensify/App pull request over the finish line! 🎉

I know there's a lot of information in our contributing guidelines, so here are some points to take note of 📝:

  1. Now that your first PR has been merged, you can be hired for another issue. Once you've completed a few issues, you may be eligible to work on more than one job at a time.
  2. Once your PR is deployed to our staging servers, it will undergo quality assurance (QA) testing. If we find that it doesn't work as expected or causes a regression, you'll be responsible for fixing it. Typically, we would revert this PR and give you another chance to create a similar PR without causing a regression.
  3. Once your PR is deployed to production, we start a 7-day timer ⏰. After it has been on production for 7 days without causing any regressions, then we pay out the Upwork job. 💰

So it might take a while before you're paid for your work, but we typically post multiple new jobs every day, so there's plenty of opportunity. I hope you've had a positive experience contributing to this repo! 😊

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @roryabraham in version: 1.1.15-18 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants