-
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
Fix/27544 Incorrect Old Dot comment formatting when viewed on New Dot #30193
Fix/27544 Incorrect Old Dot comment formatting when viewed on New Dot #30193
Conversation
This was causing a lint error during PR checks
Caught this little error currently on latest main that would've rendered 'props.framgent.text' literally instead of the actual text
<Text>{props.fragment.text}</Text> Seeing how the other cases are handled, it's possible this may have been intentional. App/src/pages/home/report/ReportActionItemFragment.js Lines 170 to 183 in 60576fe
If at all any of you, @jjcoffee or @lakchote, feel this change isn't needed, I can just revert it. It could also save on some testing. |
Just Android Native testing left to go, and the PR should be ready for review. Saved it for last because it can take a while to build. |
Tried building from Android Studio, but that took a long time to even import the files, so I switched back to using VS Code. Still waiting on the app to build on Android Native. |
Android Native has built, but the font is still bolded, for some reason. The color is grey as expected. Investigating. |
It seems that's just how a |
The styling guide says inline styles are forbidden, but I was a bit hesitant to create a helper method An inline style has been applied over here, so I thought maybe things could work as is. App/src/components/MentionSuggestions.js Line 107 in 3380266
In any case, should we go with the helper method, I could add this in getMutedColorAndNormalFontWeight: (isApprovedOrSubmittedReportActionType: Boolean) => (isApprovedOrSubmittedReportActionType ? {color: theme.textSupporting, fontWeight: 'normal'} : {}), And then replace the added style in style={[
styles.chatItemMessageHeaderSender,
props.isSingleLine ? styles.pre : styles.preWrap,
styles.getMutedColorAndNormalFontWeight(_.contains([CONST.REPORT.ACTIONS.TYPE.APPROVED, CONST.REPORT.ACTIONS.TYPE.SUBMITTED], props.actionName)),
]} |
Reviewer Checklist
Screenshots/Videos |
@Victor-Nyagudi Regarding the above, to keep things consistent with other system messages, I actually think we don't want tooltips to display here. Is it an easy fix? I think technically it's out of scope for this issue, but it might as well be fixed if it's not too big a deal! |
@jjcoffee The fragments we get from Old Dot are wrapped in I suspect this could be why the tooltip is shown on hover, though I haven't dived too deep into it. App/src/pages/home/report/ReportActionItemFragment.js Lines 155 to 169 in e6af6e9
It's EOD for me (I can still reply on mobile), so I'll take a deeper look first thing tomorrow to try figure out why the tooltip is there in the first place and what we can potentially do about it. |
…t with <Text> This is done as per @jjcoffee's suggestion. Co-authored-by: Joel Davies <joeld.dev@gmail.com>
… to ReportActionItemFragment
Hmm did you consider just adding flex-direction: row conditionally on the parent
I think it's just a bit messy to unnecessarily wrap with a |
Yes, I did explore styling the |
@Victor-Nyagudi Ah sorry, forgot about that! What about just |
No worries. Did you notice any issues when the fragments aren't wrapped in a I'm investigating as we speak. |
…L scenarios Arabic display names caused the text to start from the right side of the screen even though the system message is in English
Looks like one of the checks failed. I'll investigate first thing in the morning. |
It appears the performance test started failing after I merged These tests also seem to be fairly new. Could we try running them again to see if they're still failing? @jjcoffee We're also approaching the 9 business days mark, so this PR is now my top priority so we can get it merged before that point is reached. |
@Victor-Nyagudi Just a heads up that it's about 3 hours until my EOD. |
Got it, @jjcoffee . I'm available for all 3. |
@Victor-Nyagudi Any luck with this? |
Checking right now. |
@jjcoffee Aside from the |
@Victor-Nyagudi Nope happy with everything else! |
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.
LGTM!
Thank you for your patience and sticking with me through this one, @jjcoffee. |
Great work and collaboration both of you @Victor-Nyagudi @jjcoffee 👍 |
✋ 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 https://github.com/lakchote in version: 1.3.96-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀
|
Details
When viewing a report approved/submitted on Old Dot in New Dot, it was incorrectly formatted i.e. bolded, white, and separated on different lines.
This PR restores the grey color and regular font weight similar to other types of report actions/system messages and shows the approved/submitted report on one line.
Fixed Issues
$ #27544
PROPOSAL: #27544 (comment)
Tests
Pre-requisite: User A must have created a "Collect" workspace on Old Dot whose "Approval Mode" is set to "Submit and Approve", is the admin of the workspace, and has invited User B as an employee.
{"pageReportID":8096248714236998,"keepCollection":true}
. Copy the number afterpageReportID
.staging.new.expensify.com/r/[the long number you copied goes here not surrounded by brackets]
.Please note:
You'll notice that the copy to clipboard icon appears when you hover over the system messages from Old Dot while in New Dot, however, clicking it doesn't copy anything to the clipboard. Try doing so and pasting what you "copied" in the composer.
If in Android Web DEV (connected using your Mac's IP address and port number) you try long-pressing a message (in other chats too other than the workspace here), choose "Copy to clipboard" , and then try pasting the text in the composer, what you tried copying isn't pasted.
Offline tests
After repeating the steps in "Tests" above, turn off your wifi or go to your avatar in the top left -> preferences -> toggle force offline.
Verify the text "You final approved this report" and "User B submitted this report to you" is greyed out, has a regular font weight, and is all in one line.
QA Steps
Same as above in Tests section.
Additional Steps (after performing steps in Tests section)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop