-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
make maximum participants reached msg consistent #9144
make maximum participants reached msg consistent #9144
Conversation
@AndrewGable @tgolen @cead22 @iwiznia @flodnv I'm not sure why y'all were all automatically assigned to review this PR - feel free to if you'd like, but I feel like we don't need everyone to review 😅 (pinged in #engineering-chat to see if anyone knows what happened here: https://expensify.slack.com/archives/C03TQ48KC/p1653400316734399) |
@mananjadhav while you're testing this PR on every platform can you please include a video or screenshot on Android so we have pics from all platforms here? 👍 |
Yes, I'll do that @Beamanator |
@tgolen can we please stop that until https://github.com/Expensify/Expensify/issues/211520 is resolved? |
I am unwell and I need 1-2 days to get back to this. Sorry for the delay. |
@@ -105,6 +105,7 @@ export default { | |||
leaveRoom: 'Leave room', | |||
your: 'your', | |||
conciergeHelp: 'Please reach out to Concierge for help.', | |||
maxParticipantsReached: ({count}) => `You've selected the maximum number (${count}) of participants.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: But any specific reason we moved it as common.maxParticipantsReached
instead of messages.maxParticipantsReached
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the other texts that are commonly used are in common
object, so it made sense to add this one to common
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small question, the rest of the PR looks good. Will test it shortly. Also can you please take the latest main pull and resolve conflicts?
#### PR Reviewer Checklist- [x] I verified the PR has a small number of commits behind `main` - [x] I verified the correct issue is linked in the `### Fixed Issues` section above - [x] I verified testing steps are clear and they cover the changes made in this PR - [x] I verified the testing environment is mentioned in the test steps - [x] I verified testing steps cover success & fail scenarios (if applicable) - [x] I checked that screenshots or videos are included for tests on [all platforms](https://github.com/Expensify/App/blob/main/CONTRIBUTING.md#make-sure-you-can-test-on-all-platforms) - [x] I verified tests pass on **all platforms** & I tested again on: - [x] iOS / native - [x] Android / native - [x] iOS / Safari - [x] Android / Chrome - [x] MacOS / Chrome - [x] MacOS / Desktop - [x] I verified there are no console errors related to changes in this PR - x ] I verified proper code patterns were followed (see [Reviewing the code](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md#reviewing-the-code)) - [ ] I verified comments were added when the code was not self explanatory - [x] I verified any copy / text shown in the product was added in all `src/languages/*` files (if applicable) - [ ] I verified proper naming convention for platform-specific files was followed (if applicable) - [ ] I verified [style guidelines](https://github.com/Expensify/App/blob/main/STYLING.md) were followed - [x] I verified the JSDocs style guidelines (in [`STYLE.md`](https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs)) were followed - [x] I verified that this PR follows the guidelines as stated in the [Review Guidelines](https://github.com/Expensify/App/blob/main/PR_REVIEW_GUIDELINES.md) - [x] I verified other components are not impacted by changes in this PR (i.e. if the PR modifies a shared library or component like `Avatar`, I verified the components using `Avatar` are working as expected) - [x] I verified the UI performance was not affected (the performance is the same than `main` branch) - [ ] If a new component is created I verified that a similar component doesn't exist in the codebase @adeel0202 Changes look good and test well. Attaching the Android screenshots: cc - @Beamanator 🎀 👀 🎀 |
Sorry for the delay @adeel0202 but now there are conflicts, can you get those fixed so we can merge? 🙏 |
@Beamanator, I have resolved the conflicts. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @Beamanator in version: 1.1.74-0 🚀
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.74-2 🚀
|
Details
Consistent maximum participants reached message in Split and New Group screens
Fixed Issues
$ #8847
Tests
Press + icon
Select New group
Choose 8 participants
Press + icon
Select Split bill
Enter any amount and press Next
Choose 8 participants
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Screenshots
Web
Mobile Web
Desktop
iOS