fixed: Web - Desktop Attachments - Cannot attach a photo when the NEW ( green plus) button is pressed #9021
Conversation
melvin-bot
bot
requested review from
Luke9389 and
mananjadhav
and removed request for
a team
| @@ -83,6 +89,9 @@ class SidebarScreen extends Component { | |||
| * Selecting an item on CreateMenu or closing it by clicking outside of the modal component | |||
| */ | |||
| hideCreateMenu() { | |||
| if (window.document) { | |||
| window.document.removeEventListener('dragover', this.dragOverListener); | |||
There was a problem hiding this comment.
We need to refactor this component to have web platform-specific changes in the index.web.js.
Please check the the guidelines here: https://github.com/Expensify/App#platform-specific-file-extensions
There was a problem hiding this comment.
Thank you for your comment @mananjadhav .
When I saw that it didn't work on android and ios, I wanted to do this too. Frankly, I was afraid to repeat too much code for 10 lines. If code duplication is not a problem, I would like to do it as follows, if you see fit.
I'm a little afraid that the same codes will repeat. I wanted to get your permission before I did it.
There was a problem hiding this comment.
Thanks for the update. In these cases we should push all the common code to a component say BaseSidebarScreen. We then add/override only platform-specific functions and noops in others. This will avoid duplication and code will be cleaner.
A quick small example would look like this:
BaseSidebarScreen.js
const propTypes = {
beforeHideCreateMenu: PropTypes.func,
}
class BaseSidebarScreen {
hideCreateMenu() {
if(this.props.beforeHideCreateMenu) {
this.props.beforeHideCreateMenu();
}.
this.setState({
isCreateMenuActive: false,
});
}
}
index.web.js
class SidebarScreen {
render() {
return <BaseSidebarScreen ... />
}
}
`index.js`
class SidebarScreen {
render() {
return <BaseSidebarScreen ... beforeHideCreateMenu={() => { document.removeEventListener('dragover', this.dragOverListener); }} />
}
}
I hope this helps, do reach out if you have any other questions :)
There was a problem hiding this comment.
Thank you so much helps a lot 🙏 . I will try to do as you say. thank you so much.
There was a problem hiding this comment.
I made a change as you said in the light of the guidance. I hope you will like it @mananjadhav 🙏 👼 .
if (this.props.beforeHideCreateMenu) {
this.props.beforeHideCreateMenu();
}if (this.props.afterShowCreateMenu) {
this.props.afterShowCreateMenu(this);
}These conditions may have been extra. I need your opinion as I am not sure. Because I defined it as default props.
Thank you for your help again. I wouldn't dare to change the files 😅.
There was a problem hiding this comment.
Thank you for your help again. I wouldn't dare to change the files
Anytime. That's what we're here for :) Don't hesitate in asking any questions. Thank you for your time.
| @@ -68,6 +77,9 @@ class SidebarScreen extends Component { | |||
| this.setState({ | |||
| isCreateMenuActive: true, | |||
| }); | |||
| if (this.props.afterShowCreateMenu) { | |||
There was a problem hiding this comment.
You can remove this check.
| @@ -68,6 +77,9 @@ class SidebarScreen extends Component { | |||
| this.setState({ | |||
| isCreateMenuActive: true, | |||
| }); | |||
| if (this.props.afterShowCreateMenu) { | |||
| this.props.afterShowCreateMenu(this); | |||
There was a problem hiding this comment.
I would recommend removing this from the parameters. Instead pass a ref in the SidebarScreen/index.js
| @@ -83,6 +95,9 @@ class SidebarScreen extends Component { | |||
| * Selecting an item on CreateMenu or closing it by clicking outside of the modal component | |||
| */ | |||
| hideCreateMenu() { | |||
| if (this.props.beforeHideCreateMenu) { | |||
| * | ||
| * @param {Object} e native Event | ||
| */ | ||
| dragOverListener() { |
There was a problem hiding this comment.
Is this function required here?
| * @param {BaseSidebarScreen} baseComponent | ||
| */ | ||
| createDragoverListener= (baseComponent) => { | ||
| this.dragOverListener = this.dragOverListener.bind(baseComponent); |
There was a problem hiding this comment.
Don't pass baseComponent. Instead add a ref to BaseSidebarScreen and use that.
| import React, {PureComponent} from 'react'; | ||
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
|
|
||
| class SidebarScreen extends PureComponent { |
There was a problem hiding this comment.
I think we can change this Function component. We're not using setState and hence don't need this to be a Class Component
|
|
||
| const propTypes = { | ||
|
|
||
| /* Create Listener callback */ | ||
| afterShowCreateMenu: PropTypes.func, |
There was a problem hiding this comment.
I know I suggested this name, but generally, I avoid before or after naming conventions, and rather use onHide, etc. Let me think if we can name it better. Do let me know if you have better suggestions.
| import React, {PureComponent} from 'react'; | ||
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
|
|
||
| class SidebarScreen extends PureComponent { |
There was a problem hiding this comment.
Like the web component, this can be a Function component.
There was a problem hiding this comment.
Thanks for the changes @metehanozyurt. Looks better, have left a few comments.
Do ping if you need any help.
…ex.js using ref now
|
I'm so sorry for being late @mananjadhav.
I hope you like these changes. Thank you very much for your time and helps 🙏 👼 . |
| import React from 'react'; | ||
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
|
|
||
| export default function SidebarScreen(props) { |
There was a problem hiding this comment.
Add displayName, propTypes for SidebarScreen.
There was a problem hiding this comment.
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);
| import React, {useRef} from 'react'; | ||
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
|
|
||
| export default function (props) { |
There was a problem hiding this comment.
I am very embarrassed for the mistake I made. Please accept my apology :( .
There was a problem hiding this comment.
No worries. Happens with the best of us.
|
|
||
| /* Remove Listener callback */ | ||
| onHideCreateMenu: PropTypes.func, | ||
|
|
||
| /* Beta features list */ | ||
| betas: PropTypes.arrayOf(PropTypes.string).isRequired, |
There was a problem hiding this comment.
This can come from the sidebarPropTypes
| @@ -59,6 +64,9 @@ class SidebarScreen extends Component { | |||
|
|
|||
| const routes = lodashGet(this.props.navigation.getState(), 'routes', []); | |||
| Welcome.show({routes, showCreateMenu: this.showCreateMenu}); | |||
| if (this.props.innerRef) { | |||
There was a problem hiding this comment.
We don't need the ref here. Please check my comment below.
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
|
|
||
| const SidebarScreen = (props) => { | ||
| const BaseSidebarScreenRef = useRef(null); |
There was a problem hiding this comment.
| const BaseSidebarScreenRef = useRef(null); | |
| let baseSidebarScreen = null; |
| * Method create event listener | ||
| */ | ||
| const createDragoverListener = () => { | ||
| document.addEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu); |
There was a problem hiding this comment.
| document.addEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu); | |
| document.addEventListener('dragover', baseSidebarScreen.hideCreateMenu); |
| * Method remove event listener. | ||
| */ | ||
| const removeDragoverListener = () => { | ||
| document.removeEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu); |
There was a problem hiding this comment.
| document.removeEventListener('dragover', BaseSidebarScreenRef.current.hideCreateMenu); | |
| document.removeEventListener('dragover', baseSidebarScreen.hideCreateMenu); |
| }; | ||
| return ( | ||
| <BaseSidebarScreen | ||
| innerRef={BaseSidebarScreenRef} |
There was a problem hiding this comment.
| innerRef={BaseSidebarScreenRef} | |
| ref={el => baseSidebarScreen = el} |
There was a problem hiding this comment.
PR has made good progress. Thanks, @metehanozyurt. Few more suggestions and we should be good to merge.
…barScreen using sidebarPropTypes.
|
I made the changes based on your suggestions @mananjadhav . Thank you very much for your valuable time and support 🙏 . You can be sure that I will do my best not to make the mistakes I have made in my next works. /* Create Listener callback */
onShowCreateMenu: PropTypes.func,
/* Remove Listener callback */
onHideCreateMenu: PropTypes.func,I did not move these two properties into sidebarPropTypes. I did it like this because it is not used in native code. If you want me to do that, I can do it right away. |
|
|
||
| /* Is workspace is being created by the user? */ | ||
| isCreatingWorkspace: PropTypes.bool, | ||
| /* Create Listener callback */ |
There was a problem hiding this comment.
This could be more descriptive
|
I think the change notification did not come, so I wanted to write. /* A callback to call when we show create menu */
onShowCreateMenu: PropTypes.func,
/* A callback to call when we hide create menu */
onHideCreateMenu: PropTypes.func,If I did something else that could cause problems, I'd be happy to replace it. Thank you for your time @mananjadhav 🙏 . |
There was a problem hiding this comment.
@metehanozyurt Sorry for the delay. I've slightly changed the language of the comment in the suggestion. Rest all it looks good to me.
Also can you pull the latest main branch?
|
|
||
| /* Is workspace is being created by the user? */ | ||
| isCreatingWorkspace: PropTypes.bool, | ||
| /* A callback to call when we show create menu */ |
There was a problem hiding this comment.
| /* A callback to call when we show create menu */ | |
| /* Callback function when the menu is shown */ |
|
|
||
| ...windowDimensionsPropTypes, | ||
| /* A callback to call when we hide create menu */ |
There was a problem hiding this comment.
| /* A callback to call when we hide create menu */ | |
| /* Callback function before the menu is hidden*/ |
|
Thank you @mananjadhav I changed comments and pulled the latest main branch. |
|
Changes look fine, but I am not sure about the break in the GH action. Neither do I have an option to rerun the workflow. @Luke9389 can you help here? PR Review done 🎀 👀 🎀 |
|
Re-ran the GH action and it passed ✅ |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to production by @yuwenmemon in version: 1.1.74-2 🚀
|
Details
When user click Create icon open menu and events drag/drop wont work. This PR fix this issue.
Fixed Issues
$ #8842
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
web-Screen-Recording-2022-05-15-at-11.18.56.mp4
Mobile Web
ios-web-Screen-Recording-2022-05-15-at-11.45.22.mp4
android-web-Screen-Recording-2022-05-15-at-11.56.02.mp4
Desktop
desktop-Screen-Recording-2022-05-15-at-11.25.57.mp4
iOS
ios-native-Screen-Recording-2022-05-15-at-11.40.11.mp4
Android
android-native-Screen-Recording-2022-05-15-at-11.54.34.mp4