Style conflict resolution page #199

Tyler-Brenneman · 2017-03-27T01:03:31Z

Closes #148

Styles the conflict resolution page.

Handled sync cases:

Flagging deleted remote deck -> add to inaccessibleDecks
Flagging deleted private deck -> delete deck
Editing deleted private deck -> sync conflict

mattyu007

Looking good.

mattyu007 · 2017-03-27T23:12:15Z

Cue/app/actions/library.js

    // check if any other changes need to be synced
    for (let key in change) {
-      if((key === 'cards' && change.cards.length) || (change[key] != serverDeck[key] && !key.match("^(?:cards|uuid|action)$"))) {
+      if((key === 'cards' && change.cards.length) || (change[key] != updatedDeck[key] && !key.match("^(?:cards|uuid|action)$"))) {
        serverDeck = await LibraryApi.editDeck({


Should this be updatedDeck = ?

mattyu007 · 2017-03-28T01:36:18Z

Cue/app/common/SyncConflict.js


-  //TODO: issue #146: style this page
+type Conflict = {


Import this from actions/library instead?

mattyu007 · 2017-03-28T01:39:52Z

Cue/app/common/SelectableTextTableRow.js

-      <Text style={this.props.disabled ? styles.textDisabled : styles.text}>
-        {this.props.text}
-      </Text>
+      <View>


This view needs flex: 1 and the icon needs flex: 0 for the text to wrap and not push the icon off the screen.

Also, minor nitpick: I think the Android paddingVertical in container can be reduced to 8, the rows are spaced really far apart right now.

mattyu007 · 2017-03-28T01:50:43Z

Cue/app/common/SelectableTextTableRow.js

+        <Text style={this.props.disabled ? styles.textDisabled : styles.text}>
+          {this.props.text}
+        </Text>
+        <Text style={styles.textSecondary}>


You have to only render this conditionally based on whether this.props.subText is specified, otherwise layouts with just primary text are shifted.

mattyu007 · 2017-03-28T01:57:42Z

Cue/app/common/SyncConflict.js

+  _internetAlert = () => {
+    Alert.alert (
+      Platform.OS === "android" ? 'Failed to resolve conflicts' : 'Failed to Resolve Conflicts',
+      'Check your Internet connection and try again.')


Can just pull the recoveryMessage based on changes in #207.

mattyu007 · 2017-03-28T02:09:09Z

Cue/app/common/SyncConflict.js

+        ? 'Keep changes from "' +  conflict.serverDeck.last_update_device + '"'
+        : 'Keep server copy'
+    } else {
+      serverText = 'Keep deleted server change'


Maybe "Delete this deck" and make the subtext "Deleted on another device" or something?

mattyu007 · 2017-03-28T02:25:42Z

Cue/app/common/SyncConflict.js

+    if (interval > 1) {
+      return interval + " minutes";
+    }
+    return Math.floor(seconds) + " seconds";


I'd actually call this "just now" so the time is a little fuzzier.

mattyu007 · 2017-03-28T02:26:43Z

Cue/app/common/SyncConflict.js

        <ListView
-          onLayout={this.on_layout}
+          key={this.state.conflicts}


Does this need a key?

mattyu007 · 2017-03-28T02:29:36Z

Cue/app/reducers/library.js

-      else
-        decks[deckIndex] = serverDeck
+      } else if (updatedDeck.deleted) {
+        decks.splice(deckIndex,1)


Isn't this already handled by the inaccessibleDecks stuff in LOADED_LIBRARY?

mattyu007 · 2017-03-28T02:32:49Z

Cue/app/common/SyncConflict.js

+    return Math.floor(seconds) + " seconds";
+  }
+
+  _renderRow = (conflict) => {


mattyu007 · 2017-03-28T17:33:58Z

Cue/app/common/SyncConflict.js

      icon: CueIcons.cancel,
      onPress: () => { this.props.navigator.pop() }
    }
  }

  _getRightItems = () => {
-    if (this.state.conflicts && !this.state.conflicts.find(c => c.useServerDeck==undefined)) {
+    if (this.state.conflicts && !this.state.conflicts.find(c => c.useServerDeck==null)) {


Are we using null or undefined? This check worked anyway since:

null == undefined // true null != undefined // false null === undefined // false null !== undefined // true

I'd use === or typeof obj === 'undefined' for all checks which involve undefined values.

Also: check if !this.state.loading here.

mattyu007 · 2017-03-28T17:36:53Z

Cue/app/common/SyncConflict.js

+    if (interval >= 1) {
+      return interval + ' minute' + (interval > 1 ? 's' : '') + ' ago'
+    }
+    return ' just now'


There's an extra leading space here?

mattyu007 · 2017-03-28T17:41:26Z

Cue/app/tabs/library/LibraryHome.js

@@ -127,7 +127,8 @@ class LibraryHome extends React.Component {
          [
            {text: 'Remove', style: 'destructive'},
            {text: 'Copy', onPress: () => this.props.onCopyDeck(deck)},
-          ]
+          ],
+          { cancelable: false }


Nice catch. 👍

mattyu007 · 2017-03-28T18:04:00Z

Cue/app/common/SyncConflict.js

+
+    let content
+    if (this.state.loading) {
+       content = <ActivityIndicator/>


This needs style={{flex: 1}} to be centred.

mattyu007 · 2017-03-28T18:04:23Z

Cue/app/common/SyncConflict.js

-        label: 'Use local copy',
-        value: false
-      }
+      { label: 'Use server copy', value: true },


These aren't being used anymore, right?

mattyu007 · 2017-03-28T18:21:09Z

Cue/app/common/SyncConflict.js

+        <View style={{flex: 1}}>
+          <Text style={styles.headerText}>{headerText}</Text>
+          <ListView
+            dataSource={this.state.dataSource}


Can you add removeClippedSubviews={false} to the ListView (and also to the ListViews in LibraryListView and CardListView)? There's a long-standing RN bug where if a ListView is updated off-screen, when it reappears on the screen, the contents don't appear until the list is scrolled facebook/react-native#8607 In this case, the list can be rendered before the view is animated onto the screen, so the ListView is empty until it is scrolled.

Tyler-Brenneman · 2017-03-28T19:36:03Z

@mattyu007 Updated. Set removeClippedSubviews={false} only when platform is ios

mattyu007

Looks good. We should try another stress test to make sure scroll performance on iOS doesn't regress tremendously because of removeClippedSubviews.

mattyu007 · 2017-03-28T20:20:29Z

Closes #200.

Tyler-Brenneman added the Client label Mar 27, 2017

Tyler-Brenneman force-pushed the client/styleConflict branch from 00f2f7b to c596fba Compare March 27, 2017 01:16

mattyu007 reviewed Mar 28, 2017

View reviewed changes

Tyler-Brenneman added 2 commits March 28, 2017 04:36

initial commit

f178f52

Handle flagging deleted cards + matt's suggestions

98bc96c

Tyler-Brenneman force-pushed the client/styleConflict branch from c596fba to 98bc96c Compare March 28, 2017 08:37

Tyler-Brenneman added 2 commits March 28, 2017 04:41

change cancle name

5d45d42

fix spacing

2162a7c

Tyler-Brenneman changed the title ~~[WIP] Style conflict resolution page~~ Style conflict resolution page Mar 28, 2017

Tyler-Brenneman requested review from ClaytonPassmore, henryc132 and mattyu007 March 28, 2017 08:46

Tyler-Brenneman added 2 commits March 28, 2017 05:00

flexy

013cf19

fix edit deleted deck

fe45b5b

mattyu007 reviewed Mar 28, 2017

View reviewed changes

removeClippedSubviews

3e0b403

mattyu007 approved these changes Mar 28, 2017

View reviewed changes

Tyler-Brenneman merged commit f35610e into master Mar 28, 2017

Tyler-Brenneman mentioned this pull request Mar 28, 2017

Style sync conflict page #146

Closed

mattyu007 mentioned this pull request Mar 28, 2017

ListView contents disappear occasionally when popping from the Navigator stack #200

Closed

Tyler-Brenneman deleted the client/styleConflict branch March 29, 2017 05:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Style conflict resolution page #199

Style conflict resolution page #199

Tyler-Brenneman commented Mar 27, 2017 •

edited

Loading

mattyu007 left a comment

mattyu007 Mar 27, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

mattyu007 Mar 28, 2017

Tyler-Brenneman commented Mar 28, 2017 •

edited

Loading

mattyu007 left a comment

mattyu007 commented Mar 28, 2017

Style conflict resolution page #199

Style conflict resolution page #199

Conversation

Tyler-Brenneman commented Mar 27, 2017 • edited Loading

mattyu007 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tyler-Brenneman commented Mar 28, 2017 • edited Loading

mattyu007 left a comment

Choose a reason for hiding this comment

mattyu007 commented Mar 28, 2017

Tyler-Brenneman commented Mar 27, 2017 •

edited

Loading

Tyler-Brenneman commented Mar 28, 2017 •

edited

Loading