TwoPaneView improvements #34

kmelmon · 2020-05-14T23:33:34Z

This change polishes up some unfinished work in TwoPaneView and gets it close to feature complete.

First some changes to dualscreeninfo:
-Fixed a bug in dualscreeninfo: windowRects was returning the "gap" between the screens instead of the screens. It was also returning a negative height instead of positive.
-Added "deviceRotation" prop to make it easier for TwoPaneView to do layout according to orientation.

And some changes to TwoPaneView:
-Now listening for deviceRotation, can handle layout in both horizontal and vertical directions
-No longer using dimensions to determine how large to size each pane, instead using flex:1
-removed panePriorityVerticalSpanning prop, this was only meant for prototype
-Default props were not working. Fixed.
-Several props were incorrectly copied to state. Fixed.
-Reworked the layout logic and verified all modes work correctly.
-Ensure we always create a pane to take up layout space even if the app didn't supply a child, ensures layout is consistent for each mode.
-Simplified the sample app

Note that I'm bumping the versions on dualscreeninfo and twopaneview to 0.1.0 for a couple reasons. The first reason is I'm adding a new prop. But more importantly, uing a version of 0.0.X causes confusion for the ^ specifier in dependencies - this doesn't work as there are special rules for ^. I worked this out by going to https://semver.npmjs.com and noticing that ^0.0.1 doesn't select any published version of react-native-twopaneview, and ^0.0.2 only selects version 0.0.2. This is weird! Let's not publish any more 0.0.X versions.

Still TBD:
-Use the Hinge component
-Raise onModeChange when layout changes
-Handle threshold of 640

HomeroRR · 2020-05-15T17:21:15Z

twopaneview/src/TwoPaneView.tsx

 import React, {Fragment, Component} from 'react';
 import {
  View,
  Dimensions,


Import on Dimensions no longer needed :) #Resolved

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

HomeroRR · 2020-05-15T17:58:26Z

dualscreeninfo/src/types.ts

  y: number;
 }

+export type DeviceRotation = 'rotation0' | 'rotation90' | 'rotation180' | 'rotation270';


If we have a DeviceRotation type, why do users still need to use strings to specify the rotation?
Would be nice to have an enum that lists the rotation options so rotation0 can referred as rotation.0 instead of "rotation0" #Resolved

HomeroRR · 2020-05-15T18:01:15Z

twopaneview/src/TwoPaneView.tsx

-    }
-    return size;
+  isHorizontalOrientation() {
+    return (this.state.rotation === 'rotation0' || this.state.rotation === 'rotation180');


Again an enum on rotation would solve the need to know about the rotation strings #Resolved

+1 #Resolved

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

HomeroRR · 2020-05-15T18:24:45Z

twopaneview/src/TwoPaneView.tsx

-
 type State = {
-  dims: any,
+  windowRects: WindowRect[],


Again, do we still need windowRects? #Resolved

HomeroRR · 2020-05-15T18:27:01Z

twopaneview/src/TwoPaneView.tsx

-
    return (
-      <View style={{flexDirection: direction, width: this.state.dims.width, height:this.state.dims.height}}>
+      <View style={{flexDirection: this.isHorizontalOrientation() ? 'row' : 'column'}}>


Great, now with isHorizontalOrientation, orientation detection should be more reliable #Resolved

HomeroRR · 2020-05-15T18:33:06Z

twopaneview/src/TwoPaneView.tsx

-    let horizontal = this.state.dims.width >= this.state.dims.height;
+    // TODO - render Hinge
+    let horizontal = this.isHorizontalOrientation();
    let separatorWidth = horizontal ? DualScreenInfo.hingeWidth: '100%';


What exactly would be the use case for rendering '100%' of the separatorWidth? #Pending

When the device is in horizontal orientation, the two panes are rendered left/right. Otherwise (in vertical orientation) the two panes are rendered top/bottom. That's what this logic is for: depending on the orientation, the width is either the full width of the screen, or just the hinge width. Make sense?

Your question also was a good catch - there was a bug in that separatorHeight didn't have similar logic. This is fixed now.

I'm having trouble actually testing vertical orientation, as my drop of the emulator isn't doing a re-layout on orientation change. I'm chasing this down now.

In reply to: 425978578 [](ancestors = 425978578)

HomeroRR · 2020-05-15T18:36:45Z

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

 		}
-	private val windowRects: List<Rect>?
-		get() = mDisplayMask?.getBoundingRectsForRotation(rotation)
+	private val windowRects: List<Rect>


Good catch. #Resolved

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

thewulf7 · 2020-05-15T20:04:34Z

twopaneview/package.json

  ],
  "dependencies": {
-    "react-native-dualscreeninfo": "~0.0.1"
+    "react-native-dualscreeninfo": "0.1.0"


missing ^ #Resolved

thewulf7 · 2020-05-15T20:09:44Z

twopaneview/src/TwoPaneView.tsx

-
    return (
-      <View style={{flexDirection: direction, width: this.state.dims.width, height:this.state.dims.height}}>
+      <View style={{flexDirection: this.isHorizontalOrientation() ? 'row' : 'column'}}>


can you also move it out to separated var? Inline styles are code smell :) #Resolved

thewulf7 · 2020-05-15T20:13:54Z

twopaneview/src/TwoPaneView.tsx

    }
-    if (this.state.panePriority === PanePriority.Pane1) {
-      return this.renderPane1(this.getEntireSize());
+    if (paneMode === PaneMode.Double) {


nit: you can use switch here #Resolved

thewulf7 · 2020-05-15T20:14:20Z

twopaneview/src/TwoPaneView.tsx

  }

-  _handleDimensionsChange = (dimensions: { window: any; }) => {
+  _handleSpanningChanged = (update: DualScreenInfoPayload) => {


add private modifier pls #Resolved

thewulf7 · 2020-05-15T20:15:27Z

dualscreeninfo/src/dualscreeninfo/index.ts

 class RNDualScreenInfoModule implements IDualScreenInfoModule {
 	private mIsSpanning: boolean = false;
 	private mWindowRects: WindowRect[] = [];
+	private mRotation: DeviceRotation = 'rotation0';


Its ok since we have typecheck by Typescript #Resolved

thewulf7 · 2020-05-15T20:16:38Z

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

+			return if (boundings == null  || boundings.size == 0) {
+				listOf(windowBounds)
+			} else {
+				val hingeRect = boundings.get(0)


better to use boundings[0], and not allocate var #Resolved

thewulf7 · 2020-05-15T20:17:50Z

...nfo/android/src/main/kotlin/com/microsoft/reactnativedualscreen/dualscreen/DualScreenInfo.kt

+					val rightRect = Rect(hingeRect.right, 0, windowBounds.right, windowBounds.bottom)
+					listOf(leftRect, rightRect)
+				}
+				else {


no need for new line here #Resolved

kmelmon · 2020-05-17T20:02:01Z

@HomeroRR I'm checking this in now as I want to have this in with enough time to ferret out any issues before //build which starts on Tuesday.

Feel free to leave any additional comments and I will address this in a follow-up change.

kmelmon added 3 commits May 8, 2020 00:48

a bunch of fixes

a40c147

another batch of changes

07c2c39

update package.json versions for pubish

564bf66

kmelmon requested review from HomeroRR and thewulf7 May 14, 2020 23:38

kmelmon changed the title ~~Finish TwoPaneView~~ TwoPaneView improvements May 14, 2020

HomeroRR reviewed May 15, 2020

View reviewed changes

thewulf7 suggested changes May 15, 2020

View reviewed changes

kmelmon added 2 commits May 15, 2020 14:49

code review feedback

dc0499f

fix version typo

10c2639

thewulf7 approved these changes May 17, 2020

View reviewed changes

kmelmon merged commit f2bcf45 into microsoft:master May 17, 2020

This was referenced May 17, 2020

dualscreeninfo windowRects doesn't report correct rects #33

Closed

Fix outstanding issues from PR #5 #9

Closed

Document dualscreeninfo deviceOrientation property #41

Open

TwoPaneView improvements #34

TwoPaneView improvements #34

Uh oh!

Conversation

kmelmon commented May 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon May 15, 2020

Choose a reason for hiding this comment

Uh oh!

HomeroRR May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thewulf7 May 15, 2020 • edited by kmelmon Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmelmon commented May 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

kmelmon commented May 14, 2020 •

edited

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

HomeroRR May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

thewulf7 May 15, 2020 •

edited by kmelmon

Loading

kmelmon commented May 17, 2020 •

edited

Loading