Format whispers/welcome messages in the report action list of each chat by pecanoro · Pull Request #17527 · Expensify/App

pecanoro · 2023-04-17T18:43:40Z

This PR gives style to whispers:

Welcome message:

Whispers:

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/276393

Tests

Run CQ to add a welcome message to an announce room:

INSERT INTO reportNameValuePairs (reportID, name, value) VALUES (<reportID>, 'welcomeMessage', JSON('{"html":"Welcome to my public room!"}'));

Send yourself via DM the link to the public room and click on it.
As soon as you join the room, you should get the custom welcome message added via CQ.
Message should be coming from the policy owner.
Run CQ to make welcome message come from Concierge:

INSERT INTO reportNameValuePairs (reportID, name, value) VALUES (<reportID>, 'isWelcomeMessageFromConcierge', "true");

Repeat steps 2-3 with a different account. However, this time the message should be coming from Concierge
Make sure you can’t see the welcome message sent to the user from step 1-4 while being logged into another second account.

Verify that no errors appear in the JS console

Offline tests

Can't join a chat room offline.

QA

Send yourself via DM the link to this public room and click on it: https://new.expensify.com/r/2660055103772983
As soon as you join the room, you should get the custom welcome message like this:
Message should be coming from Concierge.
Leave a couple of messages.
Log in as another user and join the announce room as well.
As soon as you join the room, you should get the custom welcome message as well.
However, you shouldn't see the welcome message sent to the first user and only the latest one sent to you.
Log out and log in as our previous user, you can still the welcome message that was sent originally to you but not the second message sent to the user that just joined.

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

pecanoro · 2023-04-18T01:47:14Z

@shawnborton I tried to style it but I am terrible with CSS and didn't manage to make it work, can I get some help 🤗

This is the block: https://github.com/Expensify/App/pull/17527/files#diff-e6bee90bf18ae0cad1bb445ed9a9c0d989476874f1005810c0f8e93f1f39b99cR290-R307

pecanoro · 2023-04-18T01:56:46Z

@shawnborton Hmm btw, you need to run the query with one of the comments you sent to someone else so the header in the comment is visible, but you need to pull this PR as well: https://github.com/Expensify/Web-Expensify/pull/37069

UPDATE reportActions SET JSON_SET(message, '$.whisperedTo', JSON('[<recipientAccountID>]')) WHERE reportActionID=<reportActionID>;

Or maybe you know how to mimic it without it.

grgia · 2023-04-18T07:45:46Z

@pecanoro I'm happy to help with the styling!

shawnborton · 2023-04-18T11:36:17Z

Amazing, thank you @grgia

I think I see you in the Figma file already, but here is a closer look at alignment:

shawnborton · 2023-04-18T12:25:47Z

Nice! I think we always want whisper rows to have the highlight bg color too:

And then we just need to tighten up the spacing a bit.

grgia · 2023-04-18T12:50:27Z

Looking at those now!

grgia · 2023-04-18T13:29:45Z


diff --git a/src/pages/home/report/ReportActionItem.js b/src/pages/home/report/ReportActionItem.js
index 2c4414b424..6aa9d60534 100644
--- a/src/pages/home/report/ReportActionItem.js
+++ b/src/pages/home/report/ReportActionItem.js
@@ -268,6 +268,7 @@ class ReportActionItem extends Component {
                             <View
                                 style={StyleUtils.getReportActionItemStyle(
                                     hovered
+                                    || isWhisper
                                     || this.state.isContextMenuActive
                                     || this.props.draftMessage,
                                     (this.props.network.isOffline && this.props.action.isLoading) || this.props.action.error,
@@ -287,9 +288,9 @@ class ReportActionItem extends Component {
                                     needsOffscreenAlphaCompositing={this.props.action.actionName === CONST.REPORT.ACTIONS.TYPE.IOU}
                                 >
                                     {isWhisper && (
-                                        <View style={[styles.chatItem]}>
-                                            <View style={[styles.chatItemRight, styles.mr3]}>
-                                                <Icon src={Expensicons.Eye} />
+                                        <View style={[styles.flexRow, styles.pl5, styles.pt2]}>
+                                            <View style={[styles.pl6, styles.mr3]}>
+                                                <Icon src={Expensicons.Eye} small />
                                             </View>
                                             <Text style={[styles.chatItemMessageHeaderTimestamp]}>
                                                 {this.props.translate('reportActionContextMenu.onlyVisible')}
@@ -307,12 +308,16 @@ class ReportActionItem extends Component {
                                     )}
                                     {!this.props.displayAsGroup
                                         ? (
-                                            <ReportActionItemSingle action={this.props.action} showHeader={!this.props.draftMessage}>
+                                            <ReportActionItemSingle
+                                                action={this.props.action}
+                                                showHeader={!this.props.draftMessage}
+                                                wrapperStyles={[styles.chatItem, isWhisper && styles.pt1]}
+                                            >
                                                 {this.renderItemContent(hovered || this.state.isContextMenuActive)}
                                             </ReportActionItemSingle>
                                         )
                                         : (
-                                            <ReportActionItemGrouped>
+                                            <ReportActionItemGrouped wrapperStyles={[styles.chatItem, isWhisper && styles.pt1]}>
                                                 {this.renderItemContent(hovered || this.state.isContextMenuActive)}
                                             </ReportActionItemGrouped>
                                         )}
diff --git a/src/pages/home/report/ReportActionItemGrouped.js b/src/pages/home/report/ReportActionItemGrouped.js
index e811144cca..9c65afd9b6 100644
--- a/src/pages/home/report/ReportActionItemGrouped.js
+++ b/src/pages/home/report/ReportActionItemGrouped.js
@@ -6,10 +6,18 @@ import styles from '../../../styles/styles';
 const propTypes = {
     /** Children view component for this action item */
     children: PropTypes.node.isRequired,
+
+    /** Styles for the outermost View */
+    // eslint-disable-next-line react/forbid-prop-types
+    wrapperStyles: PropTypes.arrayOf(PropTypes.object),
+};
+
+const defaultProps = {
+    wrapperStyles: [styles.chatItem],
 };

 const ReportActionItemGrouped = props => (
-    <View style={[styles.chatItem]}>
+    <View style={props.wrapperStyles}>
         <View style={[styles.chatItemRightGrouped]}>
             {props.children}
         </View>
@@ -17,5 +25,6 @@ const ReportActionItemGrouped = props => (
 );

 ReportActionItemGrouped.propTypes = propTypes;
+ReportActionItemGrouped.defaultProps = defaultProps;
 ReportActionItemGrouped.displayName = 'ReportActionItemGrouped';
 export default ReportActionItemGrouped;
diff --git a/src/styles/utilities/spacing.js b/src/styles/utilities/spacing.js
index 9244552312..d56344c74b 100644
--- a/src/styles/utilities/spacing.js
+++ b/src/styles/utilities/spacing.js
@@ -370,6 +370,10 @@ export default {
         paddingLeft: 20,
     },

+    pl6: {
+        paddingLeft: 24,
+    },
+
     pl10: {
         paddingLeft: 40,
     },

grgia · 2023-04-18T13:36:11Z

Branch with styles for ya @pecanoro https://github.com/Expensify/App/compare/georgia-whisper-styles?expand=1

pecanoro · 2023-04-18T14:09:32Z

Amazing, it looks awesome! @grgia It's ok to push to this branch if you want to!

grgia · 2023-04-18T16:03:27Z

just to writeitdown, I had to modify the query slightly for testing:

UPDATE reportActions SET message = JSON_SET(message, '$.whisperedTo', JSON('[<recipientAccountID>]')) WHERE reportActionID='<reportActionID>';

…io-OnlyVisible

0xmiroslav

Code looks good. Some minor feedback:

0xmiroslav · 2023-04-20T15:51:31Z

@pecanoro does this feature work on all public rooms or limited rooms for now? Asking because it doesn't work on my public rooms. Only works with your link - https://new.expensify.com/r/2660055103772983

pecanoro · 2023-04-20T16:19:46Z

I replied in the chat, but basically, this will only work right now in public rooms where I set the message in the back end. I only set the message to the testing one. The functionality will get expanded in the future for all rooms to have it, but it's not a thing right now.

pecanoro · 2023-04-20T16:43:54Z

@0xmiroslav don't forget to fill out the checklist 🙏

0xmiroslav · 2023-04-20T16:46:44Z

BUG: "Only visible to you" text isn't translated into Spanish

bug.mov

0xmiroslav · 2023-04-20T16:51:34Z

@pecanoro please add this condition to shouldComponentUpdate in ReportActionItem

|| this.props.translate !== nextProps.translate

0xmiroslav · 2023-04-20T17:07:04Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

ios.mov

Android

0xmiroslav · 2023-04-20T17:07:34Z

@pecanoro please add this condition to shouldComponentUpdate in ReportActionItem
|| this.props.translate !== nextProps.translate

Once this is fixed, I will approve.

pecanoro · 2023-04-20T17:08:31Z

PR updated and I retested! Can you make sure I did not break it with my last commits? 😄

0xmiroslav · 2023-04-20T17:11:01Z

PR updated and I retested! Can you make sure I did not break it with my last commits? 😄

Confirmed and tested not breaking

0xmiroslav

MVP looks good so far. Thanks for great work!

MelvinBot · 2023-04-20T17:11:42Z

🎯 @0xmiroslav, thanks for reviewing and testing this PR! 🎉

An E/App issue has been created to issue payment here: #17726.

pecanoro · 2023-04-20T17:36:44Z

Thank you everyone!

OSBotify · 2023-04-20T17:41:48Z

✋ 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 · 2023-04-20T22:33:06Z

🚀 Deployed to staging by https://github.com/jasperhuangg in version: 1.3.3-0 🚀

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

OSBotify · 2023-04-21T20:48:40Z

🚀 Deployed to production by https://github.com/mountiny in version: 1.3.3-2 🚀

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

rushatgabhane · 2023-08-02T16:16:50Z

                            <View
                                style={StyleUtils.getReportActionItemStyle(
                                    hovered
+                                    || isWhisper


Hi, this is out of scope of this PR but we caused this issue - #20451 (comment)

On native, the "New" message maker is hidden behind the whisper message because it's not transparent.

pecanoro added 4 commits April 17, 2023 13:22

Add only visible label

c4b7c3d

Add whisperTo functionality

8d59ce9

Display names

51fb7e4

Fix errors

1ed1046

pecanoro self-assigned this Apr 17, 2023

pecanoro added 4 commits April 17, 2023 15:47

Use action

621d79f

Refactor code

12f3c99

Whispers are showing

73919c7

Handle only you

e5aa976

pecanoro and others added 2 commits April 17, 2023 23:18

Merge branch 'main' into rocio-OnlyVisible

46bd44c

Style

ebc3534

This comment was marked as outdated.

Sign in to view

grgia reviewed Apr 18, 2023

View reviewed changes

Comment thread src/pages/home/report/ReportActionItem.js Outdated

grgia reviewed Apr 18, 2023

View reviewed changes

Comment thread src/pages/home/report/ReportActionItem.js Outdated

Whisper styles

d6e54cc

pecanoro added 4 commits April 18, 2023 14:39

Use emails for whispers

8c26440

Merge branch 'rocio-OnlyVisible' of github.com:Expensify/App into roc…

503e0c3

…io-OnlyVisible

Style

2d4a524

Fix conditions

21745be

tgolen reviewed Apr 18, 2023

View reviewed changes

0xmiroslav reviewed Apr 20, 2023

View reviewed changes

Comment thread src/pages/home/report/ReportActionItem.js

Comment thread src/pages/home/report/ReportActionItem.js Outdated

Comment thread src/pages/home/report/ReportActionItem.js Outdated

Update shouldComponentUpdate

c405a2f

{} style

64e5b04

pecanoro dismissed stale reviews from tgolen and jasperhuangg via 64e5b04 April 20, 2023 17:07

0xmiroslav approved these changes Apr 20, 2023

View reviewed changes

melvin-bot Bot mentioned this pull request Apr 20, 2023

[Awaiting Payment April 28th] Payment issue for E/App PR#17527 #17726

Closed

tgolen approved these changes Apr 20, 2023

View reviewed changes

jasperhuangg merged commit 236761a into main Apr 20, 2023

jasperhuangg deleted the rocio-OnlyVisible branch April 20, 2023 17:34

OSBotify mentioned this pull request Apr 20, 2023

Deploy Checklist: New Expensify 2023-04-20 #17747

Closed

24 tasks

kbecciv mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-02] [HOLD for payment 2023-04-28] Chat - App crashes upon opening https://staging.new.expensify.com/r/2660055103772983 #17786

Closed

6 tasks

kbecciv mentioned this pull request Apr 23, 2023

Public room - Concierge welcome message displayed several times for same account #17871

Closed

6 tasks

rushatgabhane reviewed Aug 2, 2023

View reviewed changes

rushatgabhane mentioned this pull request Aug 2, 2023

[HOLD for payment 2023-07-26] [$1000] Chat - New font is cut if last message is from Concierge #20451

Closed

6 tasks

jjcoffee mentioned this pull request Sep 29, 2023

[HOLD for payment 2023-09-27] [$500] mWeb - Workspace - "only visible to ..." Notifier overflows with long names #26396

Closed

6 tasks

Uh oh!

Conversation

pecanoro commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues

Tests

Offline tests

QA

PR Author Checklist

Screenshots/Videos

Uh oh!

pecanoro commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pecanoro commented Apr 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grgia commented Apr 18, 2023

Uh oh!

shawnborton commented Apr 18, 2023

Uh oh!

This comment was marked as outdated.

Uh oh!

shawnborton commented Apr 18, 2023

Uh oh!

grgia commented Apr 18, 2023

Uh oh!

grgia commented Apr 18, 2023

Uh oh!

Uh oh!

grgia commented Apr 18, 2023

Uh oh!

pecanoro commented Apr 18, 2023

Uh oh!

grgia commented Apr 18, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xmiroslav left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

0xmiroslav commented Apr 20, 2023

Uh oh!

pecanoro commented Apr 20, 2023

Uh oh!

pecanoro commented Apr 20, 2023

Uh oh!

0xmiroslav commented Apr 20, 2023

Uh oh!

0xmiroslav commented Apr 20, 2023

Uh oh!

0xmiroslav commented Apr 20, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

0xmiroslav commented Apr 20, 2023

Uh oh!

pecanoro commented Apr 20, 2023

Uh oh!

0xmiroslav commented Apr 20, 2023

Uh oh!

0xmiroslav left a comment

Choose a reason for hiding this comment

Uh oh!

MelvinBot commented Apr 20, 2023

Uh oh!

pecanoro commented Apr 20, 2023

Uh oh!

OSBotify commented Apr 20, 2023

Uh oh!

OSBotify commented Apr 20, 2023

Uh oh!

OSBotify commented Apr 21, 2023

pecanoro commented Apr 17, 2023 •

edited

Loading

pecanoro commented Apr 18, 2023 •

edited

Loading

pecanoro commented Apr 18, 2023 •

edited

Loading