Add Rest of UI for Workspace Chats #7957
Conversation
…p into amal-workspace-chats-name-ui
…p into amal-workspace-chats-name-ui
|
This is ready for review, but I'm going to try and test the dang mobile platforms today if I can get the emulator working. |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Nice work here 🎉 Mostly NABs and small things.
|
|
||
| const propTypes = { | ||
| /** Whether it is a default Chat Room */ | ||
| shouldIncludeParticipants: PropTypes.bool, |
There was a problem hiding this comment.
Nice one, that comment didn't really match the prop name.
| </Text> | ||
| <Text style={[styles.textStrong]}> | ||
| {/* Use the policyExpenseChat owner's first name or their email if it's undefined or an empty string */} | ||
| {lodashGet(props.personalDetails, [props.report.ownerEmail, 'firstName']) || props.report.ownerEmail} |
There was a problem hiding this comment.
Where's the empty string come from (mentioned in the comment)? Should we at least have the firstName or ownerEmail?
There was a problem hiding this comment.
Ah yes I mean to say that if the firstName isn't set we should fall back to ownerEmail. I should be saying "Use the policyExpenseChat owner's first name or fall back to their email if it's unavailable". Could be a bit of a self-explanatory comment though, do you think I should just delete it?
There was a problem hiding this comment.
I feel an overwhelming almost painful impulse to delete comments if they don't explain something the code doesn't explain itself. But meditation is helping a lot with that.
amyevans
left a comment
amyevans
left a comment
There was a problem hiding this comment.
Lgtm, not much to add (although +1 to @marcaaron's comments on the translations)! But I'll defer approval to someone with more context / experience here! 😄
|
@amyevans @marcaaron Thanks for reviewing! I addressed/commented on your suggestions, so please give this another look when you get the chance! |
amyevans
left a comment
amyevans
left a comment
There was a problem hiding this comment.
👍 lgtm, but I'll allow other reviewers to chime in as well
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks solid. Nice job @TomatoToaster thanks for the changes.
| </Text> | ||
| <Text style={[styles.textStrong]}> | ||
| {/* Use the policyExpenseChat owner's first name or their email if it's undefined or an empty string */} | ||
| {lodashGet(props.personalDetails, [props.report.ownerEmail, 'firstName']) || props.report.ownerEmail} |
There was a problem hiding this comment.
I feel an overwhelming almost painful impulse to delete comments if they don't explain something the code doesn't explain itself. But meditation is helping a lot with that.
|
🚀 Deployed to production by @roryabraham in version: 1.1.43-2 🚀
|
@amyevans, please review when you get the chance,
CC: @marcaaron, @roryabraham
Details
Using correct subtitle for PolicyExpenseChats (a.k.a Worksapce Chats) and adding welcome text for PolicyExpenseChats as well.
Held on #7852
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/195472
Tests
Same as Web QA but done locally
PR Review Checklist
Contributor (PR Author) Checklist
mainbefore submitting my PR for review### Fixed Issuessection abovesrc/languages/*files (if applicable)Styling.md) for all style edits I madePR Reviewer Checklist
mainbefore submitting the PR### Fixed Issuessection abovesrc/languages/*files (if applicable)QA Steps
Perform this QA on staging
Click on the header and verify you see the details page like this (also be sure to click on settings to make sure it works)
Invite another user to this workspace and look for their workspace chat (It will have their name/email as the title). Verify the header looks like this:
Tested On