Skip to content
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.

Sign up for GitHub

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: task background image getting cropped #36261

Merged
merged 16 commits into from
Mar 22, 2024
Merged

fix: task background image getting cropped #36261

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
9ff4961
fix: animated background getting cropped on android
aswin-s Feb 7, 2024
8122391
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Feb 7, 2024
cd340a8
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Feb 9, 2024
e623767
fix: action item taking extra space
aswin-s Feb 9, 2024
eaa7e04
fix: clean lint
aswin-s Feb 9, 2024
e6e18f6
fix: background image horizontal shift on sensor value change
aswin-s Feb 12, 2024
977512b
refactor: move image offset values outside of component
aswin-s Feb 12, 2024
deee8d4
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Feb 13, 2024
1a076af
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Feb 14, 2024
5ab88ef
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Feb 16, 2024
310d81f
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Mar 18, 2024
c0f2995
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Mar 19, 2024
e7902fe
fix: extra space among report action items
aswin-s Mar 19, 2024
4cb39ce
fix: clamp translateY value
aswin-s Mar 19, 2024
d9fec75
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Mar 19, 2024
33bcc45
Merge remote-tracking branch 'upstream/main' into fix/issue-33922
aswin-s Mar 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/pages/home/report/AnimatedEmptyStateBackground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ import useWindowDimensions from '@hooks/useWindowDimensions';
import variables from '@styles/variables';
import CONST from '@src/CONST';

const IMAGE_OFFSET_Y = 75;
// Maximum horizontal and vertical shift in pixels on sensor value change
const IMAGE_OFFSET_X = 30;
const IMAGE_OFFSET_Y = 20;

function AnimatedEmptyStateBackground() {
const StyleUtils = useStyleUtils();
const {windowWidth, isSmallScreenWidth} = useWindowDimensions();
const illustrations = useThemeIllustrations();
const IMAGE_OFFSET_X = windowWidth / 2;

// If window width is greater than the max background width, repeat the background image
const maxBackgroundWidth = variables.sideBarWidth + CONST.EMPTY_STATE_BACKGROUND.ASPECT_RATIO * CONST.EMPTY_STATE_BACKGROUND.WIDE_SCREEN.IMAGE_HEIGHT;
Expand All @@ -37,7 +38,9 @@ function AnimatedEmptyStateBackground() {
xOffset.value = clamp(xOffset.value + y * CONST.ANIMATION_GYROSCOPE_VALUE, -IMAGE_OFFSET_X, IMAGE_OFFSET_X);
yOffset.value = clamp(yOffset.value - x * CONST.ANIMATION_GYROSCOPE_VALUE, -IMAGE_OFFSET_Y, IMAGE_OFFSET_Y);
return {
transform: [{translateX: withSpring(-IMAGE_OFFSET_X - xOffset.value)}, {translateY: withSpring(yOffset.value)}],
// On Android, scroll view sub views gets clipped beyond container bounds. Set the top position so that image wouldn't get clipped
top: IMAGE_OFFSET_Y,
transform: [{translateX: withSpring(xOffset.value)}, {translateY: withSpring(yOffset.value)}, {scale: 1.15}],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried about this change.
Please compare this branch vs staging on all platforms. There should be no 1px difference

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Especially, test sensor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is video testing sensors on physical iOS device.

ios_out.mp4

There should be no 1px difference

I've added a top offset top: IMAGE_OFFSET_Y, to prevent the image from getting clipped while tilting on Android devices. This actually pushes the image down by 40px. If you think we don't need to fix it (as this is the experience on prod at the moment), I'll leave it same as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the earlier implementation stretched out the image to 200% and then positioned the image back in view by translating the image by setting negative IMAGE_OFFSET_X. This caused issues on web while resizing. Instead in current implementation, I'm keeping image width at 100% but then scale it 15% to account for the additional span required while tilting the device.

Staging This Branch
staging current

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staszekscp what do you think about this change? Since you recently worked on this file.
Also, we're trying to fix regression caused by your PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the regression was caused by this line, so I would look there for a fix. I've added it, because I wanted to move the background image a bit upwards following designers' suggestion.

I suppose that the cropped image problem can be rather related to wrong container size, not the animation itself. The animation moves the image outside of container boundaries, and as a result it gets cropped.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback.
@aswin-s let's not touch animation logic if possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aimane-chnaif I don't see a way without modifying the animation logic. As we need to remove the '200%' width set for background image to fix the issue on web. In fact the core logic of the animation is still the same, all I have changed is using scale to position the image instead of setting 200% width and then positioning using negative margin.

We may also try generating adhoc build for this branch for extensive testing on native devices.

};
}, [isReducedMotionEnabled]);

Expand Down
4 changes: 2 additions & 2 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ function ReportActionItem(props) {
if (ReportUtils.isTaskReport(props.report)) {
if (ReportUtils.isCanceledTaskReport(props.report, parentReportAction)) {
return (
<View style={[StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth, true)]}>
<View>
<AnimatedEmptyStateBackground />
<View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}>
<ReportActionItemSingle
Expand All @@ -643,7 +643,7 @@ function ReportActionItem(props) {
);
}
return (
<View style={[StyleUtils.getReportWelcomeContainerStyle(props.isSmallScreenWidth, true)]}>
<View>
<AnimatedEmptyStateBackground />
<View style={[StyleUtils.getReportWelcomeTopMarginStyle(props.isSmallScreenWidth)]}>
<TaskView
Expand Down
2 changes: 1 addition & 1 deletion src/styles/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ function getReportWelcomeBackgroundImageStyle(isSmallScreenWidth: boolean, isMon
if (isSmallScreenWidth) {
return {
height: emptyStateBackground.SMALL_SCREEN.IMAGE_HEIGHT,
width: '200%',
width: '100%',
position: 'absolute',
};
}
Expand Down
Loading