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.

Sign up for GitHub

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

Jump to bottom

Separate report name and icon configuration from personal details #7453

Merged
merged 4 commits into from
Feb 3, 2022
Merged

Separate report name and icon configuration from personal details #7453

merged 4 commits into from
Feb 3, 2022

Conversation

sobitneupane
Copy link
Contributor

Details

Solves the issue "Private Room - Chair icon not displayed for private room created". I have tested on Web, mWeb and Android. But no on IOS and MacOS. So, I would like to request reviewer to double check for IOS and MacOS.

Fixed Issues

$ #7261

Tests

  • Create New Room (Private) and check report name and icon
  • Create New Room (Restricted) and check report name and icon
  • Create New Group and check report name and icon
  • Create New Chat and check report name and icon

QA Steps

  • Create New Room (Private) and check report name and icon
  • Create New Room (Restricted) and check report name and icon
  • Create New Group and check report name and icon
  • Create New Chat and check report name and icon

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screenshot from 2022-01-28 20-28-14
Screenshot from 2022-01-28 20-27-13
Screenshot from 2022-01-28 20-26-50
Screenshot from 2022-01-28 20-26-18

Mobile Web

Screenshot_20220128-213159_Chrome
Screenshot_20220128-213418_Chrome

Desktop

iOS

Android

Screenshot_20220128-211519_New Expensify
Screenshot_20220128-210044_New Expensify
Screenshot_20220128-205843_New Expensify

@sobitneupane sobitneupane requested a review from a team as a code owner January 28, 2022 16:13
@MelvinBot MelvinBot requested review from parasharrajat and tgolen and removed request for a team January 28, 2022 16:13
@parasharrajat
Copy link
Member

@sobitneupane What change did you make to resolve the regression over your previous changes?

@sobitneupane
Copy link
Contributor Author

sobitneupane commented Jan 29, 2022 •
edited
Loading

@sobitneupane What change did you make to resolve the regression over your previous changes?

Previously, I was trying to get reportName from details instead of formattedPersonalDetails in the following line:
https://github.com/sobitneupane/App/blob/efb1f415fcbb823a64f5d6126bb10449e5ba0bf0/src/libs/actions/Report.js#L341

parasharrajat
parasharrajat suggested changes Jan 29, 2022
View reviewed changes
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
parasharrajat
parasharrajat previously approved these changes Jan 29, 2022
View reviewed changes
Copy link
Member

@parasharrajat parasharrajat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. This PR contains logical changes which should work across platforms thus skipping checks for Mac or iOS screenshots.

Note: @sobitneupane we are planning to make IOS check mandatory so you should plan accordingly for future tasks. More details

cc: @tgolen

🎀 👀 🎀 C+ reviewed

tgolen
tgolen requested changes Jan 31, 2022
View reviewed changes
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
tgolen
tgolen requested changes Feb 2, 2022
View reviewed changes
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

Looks good! Just this one last change and I'll merge it.

src/libs/actions/Report.js Outdated Show resolved Hide resolved
@sobitneupane @tgolen
Make syntactic change
e4a8021 
Co-authored-by: Tim Golen <tgolen@gmail.com>
@tgolen tgolen merged commit 47ec8f4 into Expensify:main Feb 3, 2022
@OSBotify
Copy link
Contributor

OSBotify commented Feb 3, 2022

✋ 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 OSBotify mentioned this pull request Feb 4, 2022
Closed
22 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Feb 4, 2022

🚀 Deployed to staging by @tgolen in version: 1.1.36-0 🚀

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

@botify botify mentioned this pull request Feb 7, 2022
Closed
@OSBotify
Copy link
Contributor

OSBotify commented Feb 7, 2022

🚀 Deployed to production by @roryabraham in version: 1.1.36-0 🚀

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

@mvtglobally mvtglobally mentioned this pull request Feb 8, 2022
Closed
This was referenced Feb 8, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat left review comments

@tgolen tgolen tgolen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sobitneupane @parasharrajat @OSBotify @tgolen