-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[CP Staging] Fix: System Footer Visibility #41200
Conversation
@ishpaul777 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Edit: i'll be able to review after 10PM IST |
I've asked for a volunteer here. |
@neonbhai Please complete checklist. |
Reviewer Checklist
Screenshots/VideosAndroid: Native |
Waiting for an update here on if we should add a check for I'm wondering why would we need to show Since the new flow isn't ready yet, maybe it is cleaner to hide the footer for now? Waiting for updates before proceeding |
@neonbhai that is not needed anymore, before we added the onboarding tasks to the concierge chat but now they go to specific system chat. #Every use will have one and you can identify it by chatType property set to |
You could basically update the code so the canWriteInReport is true unless its chat report with type system |
There is no type such as |
@neonbhai @shubham1206agra it would be added here https://github.com/Expensify/App/pull/40678/files Feel free to add it now |
@neonbhai check out the pr j have linked and add the system type. You can also see that OpenApp call returns one chat with this chattype |
@neonbhai Can you provide the ETA for the changes? |
Finalizing in 30 minutes! |
Is this ready for re-review? |
It is! @shubham1206agra please take a look! |
I thought we were following this |
It looked like Line 1201 in c7b6e35
So instead of mixing with this var:
I separated it here This felt cleaner. We could change if you recommend? |
Thanks for handling |
(cherry picked from commit 3d06f77)
@@ -141,7 +142,7 @@ function ReportFooter({ | |||
/> | |||
)} | |||
{isArchivedRoom && <ArchivedReportFooter report={report} />} | |||
{!canWriteInReport && <SystemChatReportFooterMessage />} | |||
{!isAnonymousUser && !canWriteInReport && isSystemChat && <SystemChatReportFooterMessage />} |
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.
I believe the !isAnonymousUser
was not necessary here anymore, but good enough for the CP
…g-41200-1 🍒 Cherry pick PR #41200 to staging 🍒
🚀 Deployed to staging by https://github.com/francoisl in version: 1.4.67-3 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
Details
Fixed Issues
$ #41184
$ #41135
PROPOSAL: #41184 (comment) & #41135 (comment)
Tests
Offline tests
Same as Tests step
QA Steps
Same as Tests step
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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