Skip to content

Inset the full screen modal used for workspaces on large devices #3887

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
marcaaron merged 9 commits into from
Aug 9, 2021
Merged

Inset the full screen modal used for workspaces on large devices #3887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
29761f4
Inset the full screen modal used for workspaces on large devices
HorusGoul Jul 6, 2021
8ad161a
Use style variables
HorusGoul Jul 6, 2021
67a6c14
Remove unused parameters from the getNavigationDrawerStyle function
HorusGoul Jul 6, 2021
208dd34
Fix padding
HorusGoul Jul 6, 2021
ed7d06d
Add custom CardOverlay component so we can use the modalBackdrop color
HorusGoul Jul 6, 2021
6315c42
Move CardOverlay styles to the styles.js file
HorusGoul Jul 7, 2021
684d5b2
Merge branch 'main' into horus-inset-full-screen-modal
HorusGoul Jul 19, 2021
8de4c56
Solve two style issues related to the reports screen provoked by the …
HorusGoul Jul 19, 2021
703a063
Merge branch 'main' into horus-inset-full-screen-modal
HorusGoul Aug 9, 2021
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
7 changes: 7 additions & 0 deletions src/components/CardOverlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import React from 'react';
import {View} from 'react-native';
import styles from '../styles/styles';

const CardOverlay = () => <View style={styles.cardOverlay} />;

export default CardOverlay;
11 changes: 9 additions & 2 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ import Timers from '../../Timers';
import ValidateLoginNewWorkspacePage from '../../../pages/ValidateLoginNewWorkspacePage';
import ValidateLogin2FANewWorkspacePage from '../../../pages/ValidateLogin2FANewWorkspacePage';
import WorkspaceSettingsDrawerNavigator from './WorkspaceSettingsDrawerNavigator';
import spacing from '../../../styles/utilities/spacing';
import CardOverlay from '../../../components/CardOverlay';
import defaultScreenOptions from './defaultScreenOptions';

Onyx.connect({
Expand Down Expand Up @@ -253,10 +255,14 @@ class AuthScreens extends React.Component {
};
const fullscreenModalScreenOptions = {
...commonModalScreenOptions,
cardStyle: {...styles.fullscreenCard},
cardStyle: {
...styles.fullscreenCard,

@joelbettner joelbettner Jul 16, 2021

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.

Since styles.cardOverlay is being added below and has the following styles:

    cardOverlay: {
        backgroundColor: themeColors.modalBackdrop,
        position: 'absolute',
        top: 0,
        left: 0,
        width: '100%',
        height: '100%',
        opacity: 0.5,
    }

and styles.fullscreenCard has the following styles:

    fullscreenCard: {
        position: 'absolute',
        left: 0,
        top: 0,
        width: '100%',
        height: '100%',
    }

why do we need to add ...styles.fullscreenCard here? Won't all these styles be included in cardOverlay?

@HorusGoul HorusGoul Jul 19, 2021

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.

The cardOverlay is for the backdrop of the modal, and the fullscreenCard is for the content that appears on top of the backdrop. These styles are for two different components, hence the need for the two different styles.

@joelbettner joelbettner Jul 20, 2021

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.

I was just wondering if the styles would be inherited. In any case, if the cardOverlay component might be used elsewhere, it makes sense to pass the styles along and not rely on inheritence.

padding: this.props.isSmallScreenWidth ? spacing.p0.padding : spacing.p5.padding,
},
cardStyleInterpolator: props => modalCardStyleInterpolator(this.props.isSmallScreenWidth, true, props),
cardOverlayEnabled: false,
cardOverlayEnabled: !this.props.isSmallScreenWidth,
isFullScreenModal: true,
cardOverlay: CardOverlay,

@joelbettner joelbettner Jul 16, 2021

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.

Sould it make more sense just to have

cardOverlay: <View style={styles.cardOverlay} />,

Why have an entirely different file for something that is only used in this one place?

@HorusGoul HorusGoul Jul 19, 2021

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.

We could move it to the AuthScreen but, we would still need to pass it as cardOverlay: CardOverlay. I think it's best to leave it as a separate file just in case we need to use it for another modal.

};

return (
Expand All @@ -279,6 +285,7 @@ class AuthScreens extends React.Component {
// prevent unnecessary scrolling
cardStyle: {
overflow: 'hidden',
height: '100%',
},
}}
component={MainDrawerNavigator}
Expand Down
68 changes: 41 additions & 27 deletions src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import _ from 'underscore';
import PropTypes from 'prop-types';
import {createDrawerNavigator} from '@react-navigation/drawer';
import {View} from 'react-native';
import styles, {getNavigationDrawerStyle, getNavigationDrawerType} from '../../../styles/styles';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';

Expand All @@ -22,38 +23,51 @@ const propTypes = {
/** Drawer content Component */
drawerContent: PropTypes.elementType.isRequired,

/** If it's the main screen, don't wrap the content even if it's a full screen modal. */
isMainScreen: PropTypes.bool,

/** Window Dimensions props */
...windowDimensionsPropTypes,
};
const Drawer = createDrawerNavigator();

const BaseDrawerNavigator = props => (
<Drawer.Navigator
defaultStatus={props.isSmallScreenWidth ? 'open' : 'closed'}
sceneContainerStyle={styles.navigationSceneContainer}
drawerContent={props.drawerContent}
screenOptions={{
cardStyle: styles.navigationScreenCardStyle,
headerShown: false,
drawerType: getNavigationDrawerType(props.isSmallScreenWidth),
drawerStyle: getNavigationDrawerStyle(
props.windowWidth,
props.windowHeight,
props.isSmallScreenWidth,
),
swipeEdgeWidth: 500,
}}
>
{_.map(props.screens, screen => (
<Drawer.Screen
key={screen.name}
name={screen.name}
component={screen.component}
initialParams={screen.initialParams}
/>
))}
</Drawer.Navigator>
);
const BaseDrawerNavigator = (props) => {
const content = (
<Drawer.Navigator
defaultStatus={props.isSmallScreenWidth ? 'open' : 'closed'}
sceneContainerStyle={styles.navigationSceneContainer}
drawerContent={props.drawerContent}
screenOptions={{
cardStyle: styles.navigationScreenCardStyle,
headerShown: false,
drawerType: getNavigationDrawerType(props.isSmallScreenWidth),
drawerStyle: getNavigationDrawerStyle(
props.isSmallScreenWidth,
),
swipeEdgeWidth: 500,
}}
>
{_.map(props.screens, screen => (
<Drawer.Screen
key={screen.name}
name={screen.name}
component={screen.component}
initialParams={screen.initialParams}
/>
))}
</Drawer.Navigator>
);

if (!props.isMainScreen && !props.isSmallScreenWidth) {
return (
<View style={styles.navigationSceneFullScreenWrapper}>

@marcaaron marcaaron Jul 6, 2021

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.

Could we change only the styles passed to sceneContainerStyle? Why is the additional View necessary? Asking mostly because it would make this logic a bit cleaner without having to create the content variable, but not a blocker.

@HorusGoul HorusGoul Jul 7, 2021

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.

That's what I tried first, and I found that the sceneContainerStyle targets a different UI component.

Adding the View wrapper was necessary because it was the only way to wrap the Drawer and the Screen contents and apply the rounded corners to them.

@marcaaron marcaaron Jul 7, 2021

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.

Got it. Does having the extra View around everything else cause issues as well? Something like

<View style={props.isSmallScreenWidth ? styles.navigationSceneFullScreenWrapper : undefined}>
     ... other stuff

@marcaaron marcaaron Jul 7, 2021

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.

Also no worries if there is no way to keep all the JSX together. I think it's just easier to parse without the content variable.

{content}
</View>
);
}

return content;
};

BaseDrawerNavigator.propTypes = propTypes;
BaseDrawerNavigator.displayName = 'BaseDrawerNavigator';
Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/AppNavigator/MainDrawerNavigator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ const MainDrawerNavigator = (props) => {
initialParams,
},
]}
isMainScreen
/>
);
};
Expand Down
32 changes: 26 additions & 6 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1551,6 +1551,12 @@ const styles = {
height: '100%',
},

navigationSceneFullScreenWrapper: {
Comment thread
joelbettner marked this conversation as resolved.
borderRadius: variables.componentBorderRadiusCard,
overflow: 'hidden',
height: '100%',
},

invisible: {
position: 'absolute',
opacity: 0,
Expand Down Expand Up @@ -1855,6 +1861,22 @@ const styles = {
...whiteSpace.noWrap,
},

cardOverlay: {
backgroundColor: themeColors.modalBackdrop,
position: 'absolute',
top: 0,
left: 0,
width: '100%',
height: '100%',
opacity: 0.5,
},

communicationsLinkIcon: {
right: -36,
top: 0,
bottom: 0,
},

shortTermsRow: {
flexDirection: 'row',
padding: 12,
Expand Down Expand Up @@ -1999,20 +2021,18 @@ function getSafeAreaMargins(insets) {
/**
* Return navigation menu styles.
*
* @param {Number} windowWidth
* @param {Number} windowHeight
* @param {Boolean} isSmallScreenWidth
* @returns {Object}
*/
function getNavigationDrawerStyle(windowWidth, windowHeight, isSmallScreenWidth) {
function getNavigationDrawerStyle(isSmallScreenWidth) {

@marcaaron marcaaron Jul 6, 2021

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.

Out of curiosity, is this change just clean up or were the non relative width/height causing some issue?

@HorusGoul HorusGoul Jul 7, 2021

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.

Yep, the non-relative values were causing issues after I added the spacing around the modal (essentially, the modal wasn't respecting the parent's right and bottom padding), so I ended up making all the sizes relative to fix the issue.

An alternative to this was subtracting spacing size * 2 from the absolute values, but that was a worse solution IMO.

@marcaaron marcaaron Jul 7, 2021

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.

Yep, the non-relative values were causing issues after I added the spacing around the modal (essentially, the modal wasn't respecting the parent's right and bottom padding), so I ended up making all the sizes relative to fix the issue.

Got it, thanks. I think maybe in this case it would be good to add that context in a comment since a future change could undo that and the reasoning for these values is not very clear.

An alternative to this was subtracting spacing size * 2 from the absolute values, but that was a worse solution IMO.

That does sound more complicated.

return isSmallScreenWidth
? {
width: windowWidth,
height: windowHeight,
width: '100%',
height: '100%',
borderColor: themeColors.border,
}
: {
height: windowHeight,
height: '100%',
width: variables.sideBarWidth,
borderRightColor: themeColors.border,
};
Expand Down