-
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
Add animation for switch children #55478
Add animation for switch children #55478
Conversation
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!
@@ -181,6 +359,8 @@ function WorkspaceInitialPage({policyDraft, policy: policyProp, route}: Workspac | |||
newFeatureStates[key] = | |||
prevPendingFields?.[key] !== policy?.pendingFields?.[key] || isOffline || !policy?.pendingFields?.[key] ? isFeatureEnabled : currentFeatureStates[key]; | |||
}); | |||
|
|||
setHighlightedFeature(Object.keys(newFeatureStates).at(0) ?? ''); |
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.
NAB: instead of empty string we could use undefined.
@dubielzyk-expensify @thesahindia One of you needs to 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] |
Same thing as we discussed in this draft PR but the padding is off here on the Android recording: The workflow animations looks pretty great 😄 Also MacOS Desktop, iOS mWeb, and Android mWeb videos didn't upload or display properly. |
here are not these changes for accounting tabs, they will be in a separaten PR, I added the wrong recording. It is already corrected |
@thesahindia What is your eta for this review? @sumo-slonik could you please highligh in the diff the changes made to fix the issue found in staging? thanks! |
Ahh all good then. It looks good to me on the workflow page 👍 |
if you want to see what the other tabs will look like you can check PR: |
There's an issue in navigation.
Screen.Recording.2025-01-22.at.4.54.31.PM.mov |
Currently, there's a sliding animation when a newly enabled feature appears in the list. Are we okay with just highlighting the new feature instead? Current behaviour: Screen.Recording.2025-01-22.at.5.20.33.PM.movNew behaviour: Screen.Recording.2025-01-22.at.5.19.14.PM.mov |
This also occurs on https://new.expensify.com, so I think it's not due to this PR |
const [featureStates, setFeatureStates] = useState(policyFeatureStates); | ||
const [highlightedFeature, setHighlightedFeature] = useState<string | undefined>(undefined); | ||
|
||
const workspaceMenuItems: WorkspaceMenuItem[] = useMemo(() => { |
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.
@sumo-slonik could you please highligh in the diff the changes made to fix the issue found in staging? thanks!
The addition of mnemonization here eliminates the problem that caused unnecessary rerendering and caused the flag responsible for highlighting to be removed.
🚧 @mountiny has triggered a test hybrid app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
@mountiny this looks weird, ad hoc builds are failing with a surprising error: |
@thesahindia ready to check again, I hope it will be good now :D |
@sumo-slonik thanks! Can you please fix the lint? @thesahindia please feel free to review and test even with the failing action. Thanks |
Hi, it seems to me that this is a rather large PR and in the changes I added there is already a risk of regression, I would prefer to avoid correcting imports not related to my PR in order not to increase this risk unnecessarily. What do you think about this @mountiny ? |
@@ -1069,6 +1069,12 @@ function navigateWhenEnableFeature(policyID: string) { | |||
}, CONST.WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY); | |||
} | |||
|
|||
function goBackWhenEnableFeature(policyID: string) { | |||
setTimeout(() => { |
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'm a bit concerned about using setTimeout, we try to avoid it unless absolutely necessary.
cc: @mountiny
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.
is exactly the same solution as it was before in the navigate function:
function navigateWhenEnableFeature(policyID: string) {
setTimeout(() => {
Navigation.navigate(ROUTES.WORKSPACE_INITIAL.getRoute(policyID));
}, CONST.WORKSPACE_ENABLE_FEATURE_REDIRECT_DELAY);
}
which we replacing here
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.
Ah, my bad. Let's not make any changes here then.
This one needs to be updated as well. App/src/libs/actions/Policy/Tag.ts Line 695 in b59f23b
Please remove the function as well. Line 1066 in b59f23b
|
@sumo-slonik sounds fine, if all the lint issues are about the import rule then dont worry about it |
…itch_childreans # Conflicts: # src/pages/workspace/WorkspaceInitialPage.tsx
I've already resolved the conflict but I'm checking if it still works on all platforms after attaching the master |
@thesahindia |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeScreen.Recording.2025-01-29.at.5.50.00.PM.moviOS: NativeScreen.Recording.2025-01-25.at.9.54.21.PM.moviOS: mWeb SafariScreen.Recording.2025-01-25.at.10.12.42.PM.movMacOS: Chrome / SafariScreen.Recording.2025-01-25.at.10.14.39.PM.movMacOS: DesktopScreen.Recording.2025-01-25.at.10.17.30.PM.mov |
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.
Works well!
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.
The PR is large enough already so we decided not ot fix all the libs import lint errors.
@mountiny looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Explained above |
✋ 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/mountiny in version: 9.0.92-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.92-6 🚀
|
'payload' in action && | ||
action.payload && | ||
'name' in action.payload && | ||
(isSideModalNavigator(action.payload.name) || action.payload.name === NAVIGATORS.FULL_SCREEN_NAVIGATOR) |
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.
Looks like this line was missing 1 extra check which led to the causation of this issue:
Explanation of Change
These PR add animations of the appearance and disappearance of elements after the switch using reanimated.
Fixed Issues
$ #53759
Fixed also reason to reverse previous revert:
$ #55451
PROPOSAL:
Tests
Offline tests
Offline tests are not needed.
QA Steps
Same as tests
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
Screen.Recording.2025-01-21.at.12.12.57.mp4
Android: mWeb Chrome
Screen.Recording.2025-01-21.at.12.38.23.mp4
iOS: Native
Screen.Recording.2025-01-20.at.16.07.39.mp4
iOS: mWeb Safari
Screen.Recording.2025-01-20.at.16.13.57.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-01-20.at.16.17.23.mp4
MacOS: Desktop
Screen.Recording.2025-01-20.at.16.40.29.mp4