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

fixed: Web - Desktop Attachments - Cannot attach a photo when the NEW ( green plus) button is pressed #9021

Merged
merged 9 commits into from
Jun 7, 2022
Merged

fixed: Web - Desktop Attachments - Cannot attach a photo when the NEW ( green plus) button is pressed #9021

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a5bb991
dragOverListener method added to SidebarScreen
metehanozyurt May 15, 2022
7573440
document changed to window.document and conditions added for unnecess…
metehanozyurt May 15, 2022
c06b304
BaseSidebarScreen.js, index.js and index.native.js created.
metehanozyurt May 16, 2022
842b0a7
index.js and index.native.js files changed to function component, ind…
metehanozyurt May 17, 2022
c221851
sidebarPropTypes.js created,displayName and propTypes added for Sideb…
metehanozyurt May 17, 2022
dc06bce
allPolicies added to sidebarPropTypes. Fixed the use of ref. BaseSide…
metehanozyurt May 22, 2022
c853e4b
descriptive comment added.
metehanozyurt May 24, 2022
e48d2f9
comments changed
metehanozyurt Jun 2, 2022
1584251
Merge branch 'main' of https://github.com/Expensify/App into issue-8842
metehanozyurt Jun 2, 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
58 changes: 36 additions & 22 deletions src/pages/home/sidebar/SidebarScreen.js → ...idebar/SidebarScreen/BaseSidebarScreen.js
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,26 +5,33 @@ import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import PropTypes from 'prop-types';
import {withNavigation} from '@react-navigation/compat';
import styles from '../../../styles/styles';
import SidebarLinks from './SidebarLinks';
import PopoverMenu from '../../../components/PopoverMenu';
import FAB from '../../../components/FAB';
import ScreenWrapper from '../../../components/ScreenWrapper';
import Navigation from '../../../libs/Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import Timing from '../../../libs/actions/Timing';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import CONST from '../../../CONST';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import compose from '../../../libs/compose';
import * as Expensicons from '../../../components/Icon/Expensicons';
import Permissions from '../../../libs/Permissions';
import ONYXKEYS from '../../../ONYXKEYS';
import * as Policy from '../../../libs/actions/Policy';
import Performance from '../../../libs/Performance';
import * as Welcome from '../../../libs/actions/Welcome';
import styles from '../../../../styles/styles';
import SidebarLinks from '../SidebarLinks';
import PopoverMenu from '../../../../components/PopoverMenu';
import FAB from '../../../../components/FAB';
import ScreenWrapper from '../../../../components/ScreenWrapper';
import Navigation from '../../../../libs/Navigation/Navigation';
import ROUTES from '../../../../ROUTES';
import Timing from '../../../../libs/actions/Timing';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../../components/withWindowDimensions';
import CONST from '../../../../CONST';
import withLocalize, {withLocalizePropTypes} from '../../../../components/withLocalize';
import compose from '../../../../libs/compose';
import * as Expensicons from '../../../../components/Icon/Expensicons';
import Permissions from '../../../../libs/Permissions';
import ONYXKEYS from '../../../../ONYXKEYS';
import * as Policy from '../../../../libs/actions/Policy';
import Performance from '../../../../libs/Performance';
import * as Welcome from '../../../../libs/actions/Welcome';

const propTypes = {

/* Create Listener callback */
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be more descriptive

onShowCreateMenu: PropTypes.func,

/* Remove Listener callback */
onHideCreateMenu: PropTypes.func,

/* Beta features list */
betas: PropTypes.arrayOf(PropTypes.string).isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can come from the sidebarPropTypes


Expand All @@ -36,10 +43,12 @@ const propTypes = {
...withLocalizePropTypes,
};
const defaultProps = {
onHideCreateMenu: () => {},
onShowCreateMenu: () => {},
isCreatingWorkspace: false,
};

class SidebarScreen extends Component {
class BaseSidebarScreen extends Component {
constructor(props) {
super(props);

Expand All @@ -59,6 +68,9 @@ class SidebarScreen extends Component {

const routes = lodashGet(this.props.navigation.getState(), 'routes', []);
Welcome.show({routes, showCreateMenu: this.showCreateMenu});
if (this.props.innerRef) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need the ref here. Please check my comment below.

this.props.innerRef.current = this;
}
}

/**
Expand All @@ -68,6 +80,7 @@ class SidebarScreen extends Component {
this.setState({
isCreateMenuActive: true,
});
this.props.onShowCreateMenu();
}

/**
Expand All @@ -83,6 +96,7 @@ class SidebarScreen extends Component {
* Selecting an item on CreateMenu or closing it by clicking outside of the modal component
*/
hideCreateMenu() {
this.props.onHideCreateMenu();
this.setState({
isCreateMenuActive: false,
});
Expand Down Expand Up @@ -184,8 +198,8 @@ class SidebarScreen extends Component {
}
}

SidebarScreen.propTypes = propTypes;
SidebarScreen.defaultProps = defaultProps;
BaseSidebarScreen.propTypes = propTypes;
BaseSidebarScreen.defaultProps = defaultProps;

export default compose(
withNavigation,
Expand All @@ -202,4 +216,4 @@ export default compose(
key: ONYXKEYS.IS_CREATING_WORKSPACE,
},
}),
)(SidebarScreen);
)(BaseSidebarScreen);
29 changes: 29 additions & 0 deletions src/pages/home/sidebar/SidebarScreen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React, {useRef} from 'react';
import BaseSidebarScreen from './BaseSidebarScreen';

export default function (props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Name the function

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 am very embarrassed for the mistake I made. Please accept my apology :( .

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries. Happens with the best of us.

const BaseSidebarScreenRef = useRef(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const BaseSidebarScreenRef = useRef(null);
let baseSidebarScreen = null;


/**
* Method create event listener
*/
const createDragoverListener = () => {
document.addEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
document.addEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu);
document.addEventListener('dragover', baseSidebarScreen.hideCreateMenu);

};

/**
* Method remove event listener.
*/
const removeDragoverListener = () => {
document.removeEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
document.removeEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu);
document.removeEventListener('dragover', baseSidebarScreen.hideCreateMenu);

};
return (
<BaseSidebarScreen
innerRef={BaseSidebarScreenRef}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
innerRef={BaseSidebarScreenRef}
ref={el => baseSidebarScreen = el}

onShowCreateMenu={createDragoverListener}
onHideCreateMenu={removeDragoverListener}
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
/>
);
}
7 changes: 7 additions & 0 deletions src/pages/home/sidebar/SidebarScreen/index.native.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 BaseSidebarScreen from './BaseSidebarScreen';

export default function SidebarScreen(props) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add displayName, propTypes for SidebarScreen.

Copy link
Contributor Author

@metehanozyurt metehanozyurt May 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When I add propTypes I had to add this lines (for warnings) so that the required fields come . But I removed it from baseSidebarScreen.

export default compose(
    withNavigation,
    withLocalize,
    withWindowDimensions,
    withOnyx({
        allPolicies: {
            key: ONYXKEYS.COLLECTION.POLICY,
        },
        betas: {
            key: ONYXKEYS.BETAS,
        },
        isCreatingWorkspace: {
            key: ONYXKEYS.IS_CREATING_WORKSPACE,
        },
    }),
)(SidebarScreen);

// eslint-disable-next-line react/jsx-props-no-spreading
return <BaseSidebarScreen {...props} />;
}