-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add Chat with setup specialist to the accounting page. #52803
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
Conversation
@dukenv0307 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] |
@dukenv0307 I am a little stuck on the iOS Native builds, will add later |
I asked for translation into Spanish here |
@dukenv0307 i updated, please check again |
@dukenv0307 how is the review going? |
I'm reviewing. |
code looks good |
@Expensify/design ^ |
![]() @dukenv0307 i updated, please check again |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeScreen.Recording.2024-12-01.at.10.52.35.movScreen.Recording.2024-12-01.at.10.53.12.movScreen.Recording.2024-12-01.at.10.53.56.moviOS: mWeb SafariScreen.Recording.2024-12-01.at.10.43.34.movScreen.Recording.2024-12-01.at.10.46.12.movScreen.Recording.2024-12-01.at.10.47.04.mov |
![]() thanks @techievivek, It looks like we have data now |
@nkdengineer You will be updating the PR to check for the assigned guide before showing the |
@nkdengineer As we discussed here, we should check if they have a specialist assigned or not first. |
@techievivek @dukenv0307 Sorry, I’m not entirely clear. Do you mean that we will only show the text below if ![]() |
Yeah, we only want to show the link when |
@nkdengineer Do you need any more clarifications here? Feel free to DM me on Slack if you have any questions, thanks. |
Thanks @techievivek. I'm clear now. I'll give an update soon |
@dukenv0307 @techievivek I updated code changes and test steps. Please check again |
looks good ![]() @techievivek just mentioned 2 fields in guideDetails, but we have more than that here, should we add all of them @nkdengineer? |
email and calendarLink should be fine for now, rest are for internal purposes. |
@nkdengineer Can you pls update your PR? |
@dukenv0307 i updated |
@dukenv0307 Can you please do a final review here? Thanks. |
Yes, I can |
✋ 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/techievivek in version: 9.0.75-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.0.75-6 🚀
|
Explanation of Change
Fixed Issues
$ #51487
PROPOSAL: #51487 (comment)
Tests
Offline tests
same as above
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov