Skip to content

fix: iOS&Android - Troubleshoot menu cut in top and bottom after enabling and disabling "Use profiling". #58061

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

Merged
grgia merged 10 commits into from
Mar 28, 2025
Merged

fix: iOS&Android - Troubleshoot menu cut in top and bottom after enabling and disabling "Use profiling". #58061

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0e67671
fix: iOS&Android - Troubleshoot menu cut in top and bottom after enab…
Krishna2323 Mar 9, 2025
348c21a
fix max height of test tool modal.
Krishna2323 Mar 11, 2025
3331a15
add height variable for modal backdrop.
Krishna2323 Mar 11, 2025
9b01578
use shouldUseNarrowLayout.
Krishna2323 Mar 12, 2025
4e2d9f6
Merge branch 'main' into krishna2323/issue/57350
Krishna2323 Mar 13, 2025
004d90a
remove unused view.
Krishna2323 Mar 13, 2025
c13dbd6
fix scrollbar overflow.
Krishna2323 Mar 15, 2025
519f903
Merge branch 'main' into krishna2323/issue/57350
Krishna2323 Mar 15, 2025
3ca2f82
update max height of modal to use 75% of the screen
Krishna2323 Mar 15, 2025
b8e227c
remove modalContentMaxHeightPercentage from variables.
Krishna2323 Mar 16, 2025
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
18 changes: 13 additions & 5 deletions src/components/TestToolsModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React, {useMemo} from 'react';
import {View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import useIsAuthenticated from '@hooks/useIsAuthenticated';
import useLocalize from '@hooks/useLocalize';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
import useStyleUtils from '@hooks/useStyleUtils';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
Expand All @@ -16,6 +16,7 @@ import Button from './Button';
import ClientSideLoggingToolMenu from './ClientSideLoggingToolMenu';
import Modal from './Modal';
import ProfilingToolMenu from './ProfilingToolMenu';
import ScrollView from './ScrollView';
import TestToolMenu from './TestToolMenu';
import TestToolRow from './TestToolRow';
import Text from './Text';
Expand All @@ -24,10 +25,13 @@ function getRouteBasedOnAuthStatus(isAuthenticated: boolean, activeRoute: string
return isAuthenticated ? ROUTES.SETTINGS_CONSOLE.getRoute(activeRoute) : ROUTES.PUBLIC_CONSOLE_DEBUG.getRoute(activeRoute);
}

const modalContentMaxHeightPercentage = 0.75;

@grgia grgia Mar 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shall we move this const to variables?

@Krishna2323 Krishna2323 Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat thinks it's better to keep that inside the component, which is why it was moved from the variables file to the component. #58061 (comment)

@parasharrajat parasharrajat Mar 26, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This variable is not used across the app and we only use 75% max height for this modal so I kept it here.

@grgia grgia Mar 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That makes sense, but I see Variables as less of repeatable const and more as a lib for tracking hardcoded design values. ie. sizes for images might only be used 1x but we store in there. How do you see it?


function TestToolsModal() {
const {shouldUseNarrowLayout} = useResponsiveLayout();
const [isTestToolsModalOpen = false] = useOnyx(ONYXKEYS.IS_TEST_TOOLS_MODAL_OPEN);
const [shouldStoreLogs = false] = useOnyx(ONYXKEYS.SHOULD_STORE_LOGS);
const {windowWidth} = useWindowDimensions();
const {windowWidth, windowHeight} = useWindowDimensions();
const StyleUtils = useStyleUtils();
const styles = useThemeStyles();
const {translate} = useLocalize();
Expand All @@ -49,10 +53,14 @@ function TestToolsModal() {
return (
<Modal
isVisible={!!isTestToolsModalOpen}
type={CONST.MODAL.MODAL_TYPE.CENTERED_SMALL}
type={shouldUseNarrowLayout ? CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED : CONST.MODAL.MODAL_TYPE.CENTERED_SMALL}
onClose={toggleTestToolsModal}
innerContainerStyle={styles.overflowHidden}
>
<View style={[StyleUtils.getTestToolsModalStyle(windowWidth)]}>
<ScrollView
contentContainerStyle={[StyleUtils.getTestToolsModalStyle(windowWidth), shouldUseNarrowLayout && {...styles.w100, ...styles.pv0}]}

@parasharrajat parasharrajat Mar 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

On the web, we have padding of 20, and on mobile, it is 0. Can you keep vertical padding?

@parasharrajat parasharrajat Mar 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's leave it. It will take up more space.

style={{maxHeight: windowHeight * modalContentMaxHeightPercentage}}
>

@Krishna2323 Krishna2323 Mar 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Compare the heights of the modal on the web between this PR and staging. Gap from the edge of screens to the edge of modal.

@parasharrajat, its because of this line, we have a max height which has changed the height of the modal. Should we change that? I think its safer to keep this on we also.

@parasharrajat parasharrajat Mar 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What if restrict it based on percentage like 25% instead of 250px. Which one look better?

See this how it looks currently on mweb safari. I believe there is plenty of space above.

15.03.2025_23.10.07_REC.mp4

@Krishna2323 Krishna2323 Mar 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat, I think that's a great idea and looks good, I have updated the code.

Monosnap.screencast.2025-03-16.02-18-12.mp4

@grgia grgia Mar 24, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's have design confirm this one

@Krishna2323 Krishna2323 Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Expensify/design, could you please verify the visual changes here and here, especially the max height of the modal, which is set to 75% of the screen?

@dubielzyk-expensify dubielzyk-expensify Mar 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If it only influences that specific modal then that's okay with me. I'll let the other designers chime in too @Expensify/design

@shawnborton shawnborton Mar 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yup, that works for me too 👍

@dannymcclain dannymcclain Mar 27, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same

<Text
style={[styles.textLabelSupporting, styles.mt4, styles.mb3]}
numberOfLines={1}
Expand All @@ -74,7 +82,7 @@ function TestToolsModal() {
</TestToolRow>
)}
<TestToolMenu />
</View>
</ScrollView>
</Modal>
);
}
Expand Down