-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix - Add a setting for creating a different default report title in New Expensify Collect #59568
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 - Add a setting for creating a different default report title in New Expensify Collect #59568
Conversation
Waiting for the BE changes I wanted to finish up the FE part so I want comments from design perspective @shawnborton. Is there any way I can have the exact px of the spacing of the design in the OP 2025-04-03.15-58-45.mp4 |
@FitseTLT I am happy to share the Figma file with you, but can you first just try to reference other parts of the workspace editor and use the exact spacing you see from say the Workflows page? |
Yep Will do Pretty helpful guidance Thx 👍 |
Here is how it looks now 2025-04-05.00-54-00.mp4 |
This is feeling really solid to me! Let us know when it's ready for a test build and I will trigger them for us. |
Same! Looking forward to a test build to play around with it a bit more. |
Will do after we get the BE fixed 👍 |
@FitseTLT I believe we're good to resume the work here now that the BE changes are live! |
@shawnborton or @flaviadefaria We used to have a confirm modal for disabling report field feature in More features page do you think we need to have it now? |
Can you show me what that looks like? I don't mind the idea of using it here too. |
2025-04-10.18-02-31.mp4 |
Oh I really like adding that modal in! |
@suneox Can we wrap it up today ? Thx |
Yes, the checklist is finished I will just verify again after resolving the conflicts. |
BTW The failing jest is from another pr and the fix is here |
@FitseTLT I just compared this PR with the latest main branch. I think we should keep the warning and prevent enabling the report field in cases where there is an accounting connection. BeforeScreen.Recording.2025-05-06.at.20.26.46.mp4AfterScreen.Recording.2025-05-06.at.20.18.40.mp4cc: @flaviadefaria |
@suneox Added |
Thanks for the quick update! The new change set works as expected Screen.Recording.2025-05-08.at.00.40.55.mp4 |
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.
Code looks good. @FitseTLT @suneox can one of you provide a video of the new modal for when the user clicks on the toggle and is directed to go to the accounting settings? That way we can all confirm visually its what we expect.
@trjExpensify just to clarify, are you expecting that the toggle will have the "lock" icon on it too?
Yep, it's locked enabled or locked disabled when you're connected to an accounting solution. It all depends on if you're importing coding as report fields or not. But in both cases, you can't "manually" or independently manage report fields once you connect. |
P.S @FitseTLT conflicts again. 😅 |
@blimpich Conflict resolved |
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.
Great job!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
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.
This PR might have introduced bug #62065
If anyone could please take a look, thanks!
Details
Fixed Issues
$ #59065
PROPOSAL: #59065 (comment)
Tests
Custom report names
andPrevent members from changing report titles
options appear (they are moved from the Rules features options)Custom report names
Prevent members from changing report titles
and check that upgrade page is openedOffline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
2025-04-10.18-44-07.mp4
Android: mWeb Chrome
2025-04-10.18-46-40.mp4
iOS: Native
2025-04-10.19-21-19.mp4
iOS: mWeb Safari
2025-04-10.19-23-30.mp4
MacOS: Chrome / Safari
2025-04-10.19-38-14.mp4
MacOS: Desktop
2025-04-10.19-36-36.mp4