TwoPaneView #5
TwoPaneView #5
Conversation
|
|
|
@HomeroRR can you pls remove (or move to examples folder) example of Android and iOS from root folder of TwoPaneView - package that will be downloaded should only contain code that we be used without any examples :) |
|
@HomeroRR thanks, can you also convert your JS code to TS ?:) |
thewulf7
left a comment
thewulf7
left a comment
There was a problem hiding this comment.
Pls have a look on the comments
twopaneview/examples/TwoPaneView.js
Outdated
| const children = React.Children.toArray(this.props.children); | ||
| if (children.length > 0) { | ||
| return ( | ||
| <View key="pane1" style={{width:size.width, height:size.height}}> |
There was a problem hiding this comment.
why would you use width and height and Dimensions in General?
I believe the only way is either to use flexbox and do whole sizing natively. @kmelmon can you comment on this?
There was a problem hiding this comment.
This is somewhat complex to get right. Will take this offline to discuss.
In reply to: 391525771 [](ancestors = 391525771)
There was a problem hiding this comment.
After discussing, I think all that's needed is to set flex:1 for both pane1 and pane2.
In reply to: 392512149 [](ancestors = 392512149,391525771)
twopaneview/examples/TwoPaneView.js
Outdated
| Dimensions.removeEventListener('change', this._handleDimensionsChange); | ||
| } | ||
|
|
||
| _handleDimensionsChange = dimensions => { |
There was a problem hiding this comment.
you dont need it since DualScreenInfo updating isSpanning automatically and you need only to subscribe there
twopaneview/TwoPaneView.js
Outdated
| Dimensions.removeEventListener('change', this._handleDimensionsChange); | ||
| } | ||
|
|
||
| _handleDimensionsChange = dimensions => { |
There was a problem hiding this comment.
again - just use subscription to DualScreenInfo event
twopaneview/TwoPaneView.js
Outdated
| const children = React.Children.toArray(this.props.children); | ||
| if (children.length > 0) { | ||
| return ( | ||
| <View key="pane1" style={{width:size.width, height:size.height}}> |
| } | ||
| } | ||
|
|
||
| renderSeparator() { |
There was a problem hiding this comment.
there is Hinge component for that
twopaneview/examples/package.json
Outdated
| "react-native-vector-icons": "^6.6.0", | ||
| "react-navigation": "^4.0.10", | ||
| "react-navigation-stack": "^2.0.16", | ||
| "react-native-windows": "^0.60.0-vnext.127" |
There was a problem hiding this comment.
"react-native-windows": "^0.60.0-vnext.127" [](start = 4, length = 43)
This isn't compatible with react-native 0.61.5. Does the windows sample actually work? If so I think you want to take a dependency on 0.61.0-beta.57 #Closed
...w/examples/windows/DevDay2020KeynoteDemoReactNative/DevDay2020KeynoteDemoReactNative.vcxproj
Outdated
Show resolved
Hide resolved
twopaneview/examples/windows/DevDay2020KeynoteDemoReactNative/CustomUserControlViewManager.cpp
Outdated
Show resolved
Hide resolved
twopaneview/TwoPaneView.js
Outdated
|
|
||
| render() { | ||
|
|
||
| let direction ='row'; |
There was a problem hiding this comment.
let direction ='row'; [](start = 4, length = 21)
There's no logic here to handle vertical orientation. Have you tested rotating the device to vertical?
| } | ||
|
|
||
| renderSeparator() { | ||
| let horizontal = this.state.dims.width >= this.state.dims.height; |
There was a problem hiding this comment.
let horizontal = this.state.dims.width >= this.state.dims.height; [](start = 4, length = 65)
This isn't a reliable way to detect the orientation of the device. We can discuss offline.
There was a problem hiding this comment.
You are right and personally I do not like this hack, but could not find a better way to detect this. Please, I am open to better suggestions if you now of a better way to detect orientation.
There was a problem hiding this comment.
I can think of a couple ways to detect the orientation:
- It can be deduced from the windowRects coming from DualScreenInfo. If the device is horizontal, windowRects[1] will have a left that is != windowRects[0], and similarly if it's vertical, windowRects[1] will have a top that's != windowRects[0]. Of course there will only be two windowRects in the array when running on a dual-screen device, but this info should only be needed in that scenario.
- There's a community module called react-native-orientation. I think I would prefer option 1 to avoid another dependency, but I thought I should mention it.
You can fix this later BTW as what you have will work for all known devices at this point in time. If you decide to fix it later, just file an issue and we can track it.
|
in twopaneview/.npmignore, you'll want to add entries here, otherwise everything underneath the twopaneview directory will be published to npm. At the least you'd want to exclude the examples sub-directory. There may be others as well. #Closed |
|
|
||
| export class TwoPaneView extends Component<Props, State> { | ||
| state: State = { | ||
| dims: Dimensions.get('window'), |
There was a problem hiding this comment.
dims: Dimensions.get('window'), [](start = 4, length = 31)
Discussed offline. There are still a bunch of changes needed, like to stop using Dimensions, which can be done as follow-up changes. Filed #9 to track.
There was a problem hiding this comment.
Right, eventually we would not need Dimensions. However, I am still having trouble getting the logic right with windowRects as exposed in DualScreenInfo
twopaneview/src/TwoPaneView.tsx
Outdated
| const TPV_Orientation: PaneOrientationTypes = { | ||
| Horizontal: 'horizontal', | ||
| Vertical: 'vertical', | ||
| }; |
There was a problem hiding this comment.
I'm not an expert on typescript, but it seems like this definition is unnecessary. I think what we want is to define this as an enum in the .ts file, and then use the type directly from within TwoPaneView code. This should allow us to remove TPV_Orientaiton altogether.
Same comment applies toTPV_PanePriority and PaneMode.
#Closed
There was a problem hiding this comment.
That actually makes so much more sense. Thanks for pointing that out! Just pushed the change. #Closed
No description provided.