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 image looks cropped In the Preview page #8804

Merged
merged 14 commits into from
May 17, 2022
Merged

Fix image looks cropped In the Preview page #8804

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
8dba8e6
fix not proper image height on native
mollfpr Apr 27, 2022
1917af2
remove the exceed space on android when using attachment modal
mollfpr Apr 27, 2022
53cbcf6
add comment to give some explanation
mollfpr Apr 27, 2022
ac4511e
using getPlatform() instead rn Platform
mollfpr Apr 29, 2022
cee6581
Update src/styles/getModalStyles.js
mollfpr May 3, 2022
babaa26
refactor getModalStyles func to "shim"
mollfpr May 4, 2022
cc0b570
fix conflict
mollfpr May 4, 2022
bb8a612
function to get shouldAddTopSafeAreaPadding by modal type
mollfpr May 5, 2022
c9952ee
revert back the getModalStyles and use getShouldAddTopSafeAreaPadding…
mollfpr May 5, 2022
29b0ad8
getModalStyles to library
mollfpr May 11, 2022
17e1d3f
add default value
mollfpr May 11, 2022
ce04075
move to styles folder and rename the base function
mollfpr May 13, 2022
28f509e
rename base styles and add comment
mollfpr May 13, 2022
8beeb85
Update src/styles/getModalStyles/index.android.js
mollfpr May 16, 2022
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: 8 additions & 1 deletion src/components/ImageView/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class ImageView extends PureComponent {
imageWidth: undefined,
imageHeight: undefined,
interactionPromise: undefined,
containerHeight: undefined,
};

// Use the default double click interval from the ImageZoom library
Expand Down Expand Up @@ -72,7 +73,7 @@ class ImageView extends PureComponent {
let imageWidth = width;
let imageHeight = height;
const containerWidth = Math.round(this.props.windowWidth);
const containerHeight = Math.round(this.props.windowHeight - variables.contentHeaderHeight);
const containerHeight = Math.round(this.state.containerHeight);

const aspectRatio = Math.min(containerHeight / imageHeight, containerWidth / imageWidth);

Expand Down Expand Up @@ -134,6 +135,12 @@ class ImageView extends PureComponent {
styles.overflowHidden,
styles.errorOutline,
]}
onLayout={(event) => {
const layout = event.nativeEvent.layout;
this.setState({
containerHeight: layout.height,
});
}}
>
<Image
source={{uri: this.props.url}}
Expand Down
15 changes: 6 additions & 9 deletions src/styles/getModalStyles.js → src/styles/getModalStyles/getModalStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import CONST from '../CONST';
import colors from './colors';
import variables from './variables';
import themeColors from './themes/default';
import CONST from '../../CONST';
import colors from '../colors';
import variables from '../variables';
import themeColors from '../themes/default';

export default (type, windowDimensions, popoverAnchorPosition = {}, containerStyle = {}) => {
export default ({shouldModalAddTopSafeAreaPadding = {}}) => (type, windowDimensions, popoverAnchorPosition = {}, containerStyle = {}) => {
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 I modified the current function with a new function that accept parameters that can be determined what modal type should apply the top padding.

The previous code has shouldAddTopSafeAreaPadding returning true on CONST.MODAL.MODAL_TYPE.CENTERED, CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE, and CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED. For android we can specified CONST.MODAL.MODAL_TYPE.CENTERED and CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE no to add top padding by returning shouldAddTopSafeAreaPadding with false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @Santhosh-Sellavel @AndrewGable

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mollfpr

I think shouldAddSafeAreaPadding could be a library where computations can happen in platform-specific files.
Could be added inside the getModalStyle folder itself.

Any thoughts @AndrewGable?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Santhosh-Sellavel Can you outline your suggested fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Santhosh-Sellavel Can you outline your suggested fix?

Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@AndrewGable

Suggestion 1

Could be as simple as the library name shouldAddSafeAreaPadding where index.ios.js returns true and index.js returns false. And call it from the appropriate line (as per the proposed solution).

Suggestion 2

Could be as simple as the library name shouldAddSafeAreaPadding where
index.ios.js
index.android.js
index.js

returns appropriate boolean value based on MODAL_TYPE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think suggestion 2 will be good solution.

const {isSmallScreenWidth, windowWidth} = windowDimensions;

let modalStyle = {
Expand All @@ -16,7 +16,7 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, containerSty
let animationOut;
let hideBackdrop = false;
let shouldAddBottomSafeAreaPadding = false;
let shouldAddTopSafeAreaPadding = false;
const shouldAddTopSafeAreaPadding = shouldModalAddTopSafeAreaPadding[type] || false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This where shouldAddTopSafeAreaPadding will have according to what value we want from specific platform.


switch (type) {
case CONST.MODAL.MODAL_TYPE.CONFIRM:
Expand Down Expand Up @@ -83,7 +83,6 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, containerSty
swipeDirection = ['down', 'right'];
animationIn = isSmallScreenWidth ? 'slideInRight' : 'fadeIn';
animationOut = isSmallScreenWidth ? 'slideOutRight' : 'fadeOut';
shouldAddTopSafeAreaPadding = true;
break;
case CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE:
// A centered modal that cannot be dismissed with a swipe.
Expand Down Expand Up @@ -114,7 +113,6 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, containerSty
swipeDirection = undefined;
animationIn = isSmallScreenWidth ? 'slideInRight' : 'fadeIn';
animationOut = isSmallScreenWidth ? 'slideOutRight' : 'fadeOut';
shouldAddTopSafeAreaPadding = true;
break;
case CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED:
modalStyle = {
Expand Down Expand Up @@ -196,7 +194,6 @@ export default (type, windowDimensions, popoverAnchorPosition = {}, containerSty
};
swipeDirection = undefined;
shouldAddBottomSafeAreaPadding = true;
shouldAddTopSafeAreaPadding = true;
break;
default:
modalStyle = {};
Expand Down
8 changes: 8 additions & 0 deletions src/styles/getModalStyles/index.android.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import CONST from '../../CONST';
import getModalStyles from './getModalStyles';

export default getModalStyles({
shouldModalAddTopSafeAreaPadding: {
[CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED]: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only need to set CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED to true since that's the only thing that doesn't no break on android.

},
});
10 changes: 10 additions & 0 deletions src/styles/getModalStyles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import CONST from '../../CONST';
import getModalStyles from './getModalStyles';

export default getModalStyles({
shouldModalAddTopSafeAreaPadding: {
[CONST.MODAL.MODAL_TYPE.CENTERED]: true,
[CONST.MODAL.MODAL_TYPE.CENTERED_UNSWIPEABLE]: true,
[CONST.MODAL.MODAL_TYPE.RIGHT_DOCKED]: true,
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This are the default value before refactoring.