23298 - Desktop - Unable to move click>dragging the header in desktop app #24098
Conversation
melvin-bot
bot
requested review from
Santhosh-Sellavel
and removed request for
a team
|
@Santhosh-Sellavel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| dataSet={{id: 'pressable'}} | ||
| className="ASDASD" |
There was a problem hiding this comment.
Why do we need this?
There was a problem hiding this comment.
oh my bad, I accidentally committed it
|
@Skalakid Did some testing, mostly works well. But I noticed a weird issue clicking above the header area doesn't dismiss the RHN, post this change. Screen.Recording.2023-08-04.at.12.12.35.AM.mov |
|
@Santhosh-Sellavel I removed unnecessary code and fixed the bug you mentioned |
Reviewer Checklist
Screenshots/VideosDesktopScreen.Recording.2023-08-07.at.1.06.30.AM.movWebScreen.Recording.2023-08-07.at.1.09.07.AM.mov |
|
bump @roryabraham! |
There was a problem hiding this comment.
This seems like it leverages an internal implementation detail of react-native-web and thus is going to be unstable / subject to change w/o warning.
Instead, can we:
- Use data attributes instead of ID for
drag-area? Technically HTML documents aren't supposed to have multiple elements with the same ID, right? - Add a custom data attribute in
GenericPressablesuch that allPressablecomponents in our app are guaranteed to have that attribute unless we change/remove it, instead of leveraging thetouchActionclass that's currently added to pressables by react-native-web?
|
@roryabraham agreed! I've added the requested changes |
|
LGTM, @Santhosh-Sellavel can you re-review please since there have been significant changes? |
|
Can not drag while RHN is open while dragging using a header area not part of RHN? It was working before this PR Screen.Recording.2023-08-09.at.5.04.54.AM.mov |
| @@ -71,6 +71,7 @@ function ThreePaneView(props) { | |||
| onPress={() => props.navigation.goBack()} | |||
| accessibilityLabel={translate('common.close')} | |||
| accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} | |||
| noDragArea | |||
There was a problem hiding this comment.
This change causing this issue
@Santhosh-Sellavel @roryabraham This bug appears because, in the latest Electron version, Pressable components can't be both clickable and draggable. The dimmed dismiss modal area takes the whole space on the left and it is a Pressable. Before adding <View style={[styles.flex1, {paddingTop: 30}]}>
<PressableWithoutFeedback
style={[styles.flex1]}
onPress={() => props.navigation.goBack()}
accessibilityLabel={translate('common.close')}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON}
noDragArea
/>
</View>By adding paddingTop there would be draggable area at the top and a clickable area below it. What do you think about it? Should we add it or leave it like it is? |
|
@Skalakid Adding a If so let's go with it if not we just have to pick one between the two |
|
@Santhosh-Sellavel Unfortunately no, it will only improve #24098 (comment) since there you could drag on the same area that was under the dimmed area. With the paddingTop you will be able to drag only at the top of the window but everywhere else you will dismiss the modal. What do you think about it? Exampledesktop2.mov |
|
@Santhosh-Sellavel @roryabraham The only problem with |
|
I've added similar solution that works on all platforms |
| <View style={[styles.flex1, styles.flexColumn]}> | ||
| {/* The PressableWithoutFeedback with 30px height was added to make whole dimmed area pressable | ||
| on all web devices, but aslo make the top of the desktop apps draggable. | ||
| Please remember that in latestest Electron update, buttons can't be both draggable and clickable */} | ||
| <PressableWithoutFeedback | ||
| style={[styles.w100, {height: 30}]} | ||
| onPress={() => props.navigation.goBack()} | ||
| accessibilityLabel={translate('common.close')} | ||
| accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} | ||
| /> | ||
| <PressableWithoutFeedback | ||
| style={[styles.flex1]} | ||
| onPress={() => props.navigation.goBack()} | ||
| accessibilityLabel={translate('common.close')} | ||
| accessibilityRole={CONST.ACCESSIBILITY_ROLE.BUTTON} | ||
| noDragArea | ||
| /> | ||
| </View> |
There was a problem hiding this comment.
@roryabraham are we good with this?
There was a problem hiding this comment.
If not we can go with just removing noDragArea and have a draggable area like presented here
| {/* The PressableWithoutFeedback with 30px height was added to make whole dimmed area pressable | ||
| on all web devices, but aslo make the top of the desktop apps draggable. | ||
| Please remember that in latestest Electron update, buttons can't be both draggable and clickable */} | ||
| <PressableWithoutFeedback |
There was a problem hiding this comment.
So just to be clear, does this mean that this PressableWithoutFeedback w/ static 30px height is draggable but not clickable?
There was a problem hiding this comment.
(btw if we go with this approach we need to move the style to be in styles.js instead of inline)
There was a problem hiding this comment.
@roryabraham yes but only on the desktop app. I put two Pressables to make everything behave like it is one big Pressable (like before it was before) on other devices. But on the desktop app it will have 30px of draggable, not clickable bar at the top
|
@roryabraham bump! |
Skalakid
force-pushed
the
23298-unable-to-move-desktop-window
branch
from
1a1edea to
309828f
Compare
|
@roryabraham bump |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.59-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.60-0 🚀
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.3.60-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.59-5 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.60-3 🚀
|
Details
Fixed Issues
$ #23298
PROPOSAL: #23298 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly 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)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
N/A
Mobile Web - Chrome
N/A
Mobile Web - Safari
N/A
Desktop
desktop.mov
iOS
N/A
Android
N/A