Rough match / chat #31

benjaminben · 2017-10-27T01:21:15Z

benjaminben · 2017-10-27T09:11:10Z

So yea this is the least dry shit ever, my first thought was maybe using the same RecsView and RecInfoView components with flags for alternative rendering in this context, but then the whole writing code.thats easier to delete and all.... just seemed quicker to copy pasta for now at least.

That led to having this "MatchView" thing that oversees both chat and the match's profile - trying to anticipate tighter integration between profiles and chat, e.g being able to comment on a specific part of a profile and have that comment immediately reflected in the chat. I also really wanted to have the profile blurred behind the chat bc I think it might drive engagement? Sometimes I'm.a goldfish and forget who I'm talking or why I gaf in the first place 😶 anyway that's why there's only one scene at the store level but two "views" ("Profile" and "Chat") attached to the scene. There's probably a better way to do p much everything!

neilsarkar · 2017-10-31T09:49:35Z

Ok so there's two fairly meaty refactors we'll want to do to clean things up before merging, because the responsibilities of the components are a little muddled and are going to make this part of the codebase harder to revisit.

First off, we should extract components/Profile.js with the common rendering concerns of MatchInfoView and RecsInfoView, there are details as to how to do that here: #31 (comment)

Secondly, currently the responsibilities for interpreting the state of scene are broken down like this:

containers/Stage.js: All scene changes besides match chat and match info
containers/Match.js: Scene changes between match chat and match info
components/MatchInfoView.js: Scene change animations between match and chat
components/ChatView.js: Scene change animations between match and chat

In terms of the separation of concerns, ideally all the responsibilities for interpreting scene would be assigned to Stage.js, like so:

Stage.js: All scene changes and animation transitions between scenes

In order to support this, we'll need to make Stage.js a little smarter.

It will need to be aware of the scene to be shown
It will need to be aware of the previous scene that was being shown
It will need to be aware of the desired transition between those scenes.

We can accomplish the first two just by using this.setState in componentWillReceiveProps (don't use componentWillUpdate since we need to set the state https://stackoverflow.com/questions/29873730/react-lifecycle-methods-understanding)

In order to be aware of the transition, we can add an animation key to the object we are sending to the scene reducer.

We'll also need to add some extra markup to achieve what we want:

We'll need a containing View around the next scene and a containing Animated.View around the previous scene
We'll need to interpret the scene transition from mapStateToProps in componentWillReceiveProps and do some Animated.timing stuff

Fortunately there is a good blueprint for this in the Stage.js from eggpeg. It might be a little overwhelming at first but here's the relevant bits:

Set next and previous scene, initiate animated transition if there is one:

https://github.com/superseriouscompany/eggpeg/blob/master/app/components/Stage.js#L37

Containing view for current scene

https://github.com/superseriouscompany/eggpeg/blob/master/app/components/Stage.js#L97

Containing view for previous scene

https://github.com/superseriouscompany/eggpeg/blob/master/app/components/Stage.js#L102

Helper method to avoid duplicating the same switch statement twice

https://github.com/superseriouscompany/eggpeg/blob/master/app/components/Stage.js#L138

neilsarkar

See the main comment and the compositional refactor in these comments

neilsarkar · 2017-10-31T08:51:07Z

app/components/ChatView.js

+      this.state.topValue,
+      {
+        toValue: headerHeight,
+        duration: 333,


discussion: one thing I try to do with animation timings is have them all in the same file. see https://github.com/superseriouscompany/eggpeg/blob/master/app/config.js for an example (I would actually make it its own object). That way when you want to change animation timings you can do it all from the same place and verify that overall the differences in speeds make sense at a glance.

neilsarkar · 2017-10-31T08:54:35Z

app/components/ChatView.js

+    }
+  }
+
+  componentWillUpdate(nextProps, nextState) {


discussion: I haven't quite figured out where animation stuff should go. on the one hand, it feels like a view concern, on the other hand it requires maintaining state which ideally a view would not require. I've done it where the animation stuff is handled by the container and the value is passed in as a prop, but I'm not totally happy with that easier since in the container sometimes you're thinking about api calls and data and other times you're thinking about animations and the frontend.

after talking it through I think we should leave animation as the responsibility of the view component and just be ok with not having pure functional components when we need animations

actualllly I think my uneasiness with this is part of a bigger issue with the way responsibilities are distributed in the system. will leave a long note about relying on Stage for the transition

neilsarkar · 2017-10-31T08:56:35Z

app/components/ChatView.js

-          )}
-          />
+const {width, height} = Dimensions.get('window')
+const headerHeight = 63


this should be exported from somewhere common like app/styles/dimensions.js but since you're getting rid of the header, you can ignore for now

neilsarkar · 2017-10-31T09:05:12Z

app/components/HeaderView.js

@@ -9,7 +9,21 @@ import {
 } from 'react-native'

 export default function(props) {
-  return (
+  return (props.scene.name === 'Match' ?


we talked about how this should stay more consistent but since you're changing the header and navigation a bunch, don't worry about changing it here.

neilsarkar · 2017-10-31T09:24:27Z

app/components/MatchInfoView.js

+          :
+          null
+        }
+        <LinearGradient colors={['rgba(0,0,0,0)', 'rgba(0,0,0,0.6)']}


I think this is the lack of dryness you were referring to (the wetness, if you will)?

It's a good instinct to think that this can be cleaner and I'm really glad that you just copy-pasta'd it and threw it up so that we could discuss.

Inheritance is a bit of an anti-pattern with React. It seems like in this case you would want to use composition. In general, I prefer composition over inheritance https://stackoverflow.com/questions/49002/prefer-composition-over-inheritance

In this case, the shared code is between RecInfoView and MatchInfoView, right? And the only difference between the two is that RecInfoView has like and pass buttons?

What I would do is a two step refactor:

First:

Extract <LinearGradient and all of its children from 'components/RecInfoView.js' to a new functional view components/ProfileView.js

Replace all instances of props.recs[props.index] in the new file with props.user

Use <ProfileView user={props.recs[props.index]} /> in RecsInfoView

Add a switch for whether to show the buttons or not, like { !props.hideButtons ? ... : null }

Use <ProfileView user={props.user} hideButtons={true} /> in MatchInfoView

Second:

If possible, put the buttons outside of the profile view in RecsInfoView. If it's annoying to do it that way, just leave as is.

An example of an extracted, configurable component we used in multiple places in eggpeg is here: https://github.com/superseriouscompany/eggpeg/search?utf8=%E2%9C%93&q=RainbowBar&type=

neilsarkar · 2017-10-31T09:24:51Z

app/components/MatchInfoView.js

+
+export default MatchInfoView
+
+const style = {


should always use StyleSheet.create when possible https://stackoverflow.com/questions/38958888/react-native-what-is-the-benefit-of-using-stylesheet-vs-a-plain-object

neilsarkar · 2017-10-31T09:26:33Z

app/containers/Header.js

@@ -32,6 +32,27 @@ function mapDispatchToProps(dispatch) {

    showRecs: function() {
      dispatch({type: 'scene:change', scene: 'Recs'})
+    },
+
+    showMatch: function(user) {


why do these take a user argument?

neilsarkar · 2017-10-31T09:26:59Z

app/containers/Header.js

+    goBack: function(user) {
+      let destination = ownProps.view === 'Profile' ?
+                        {name: 'Match', view: 'Chat'} :
+                        'Matches'


this should be an object with a name key

it is? if the ternary resolves true, otherwise letting the name key be assigned by the store logic?

neilsarkar · 2017-10-31T09:28:22Z

app/containers/Match.js

+
+  onSend(messages = []) {
+    console.warn(messages[0])
+    api(`/matches/${this.props.userId}/messages`, {


confused as to why we're hitting the API twice for messages? shouldn't this only be in Chat

I see, you're using ChatView...why not import ./Chat? There's no rule about having only one container per Scene.

neilsarkar · 2017-10-31T09:28:57Z

app/containers/Stage.js

-    : props.scene == 'Chat' ?
-      <Chat userId={props.params.userId} myId={'HJyOmP2qW'}/>
+    : props.scene == 'Match' ?
+      <Match userId={props.params.userId} myId={'HyeNFOS6W'}/>


lewlz, ya after you add the user store we can get this from mapStateToProps

neilsarkar · 2017-10-31T18:36:16Z

Oh forgot I made the store smart enough to check for a string. Carry on

On Wed, Nov 1, 2017 at 2:54 AM Ben ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/containers/Header.js <#31 (comment)> : > + }, + + showMatch: function(user) { + dispatch({ + type: 'scene:change', + scene: { + name: 'Match', + view: 'Profile', + } + }) + }, + + goBack: function(user) { + let destination = ownProps.view === 'Profile' ? + {name: 'Match', view: 'Chat'} : + 'Matches' it is? if the ternary resolves true, otherwise letting the name key be assigned by the store logic? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#31 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFEpZSiRBANjz6JqAidrI0FIq8SQXUeks5sx17OgaJpZM4QIbPy> .

-- Neil

neilsarkar · 2017-11-01T01:29:36Z

app/services/dimensions.js

@@ -0,0 +1,5 @@
+import { Dimensions } from 'react-native'


sorry, I meant headerHeight, not sure how much benefit we'd get from something like this

but since it's in a bundled commit, don't worry about reverting it manually

neilsarkar · 2017-11-01T01:32:22Z

app/components/ProfileView.js

+        {
+          props.hideButtons ? null :
+          <View style={[style.tray]}>
+            <TouchableOpacity style={[style.bubble]} onPress={() => props.pass(props.recs[props.index].id)}>


this should also use props.user

neilsarkar · 2017-11-01T01:34:08Z

app/components/ProfileView.js

+                     resizeMode='contain'
+                     source={require('../images/x.png')} />
+            </TouchableOpacity>
+            <TouchableOpacity style={[style.bubble]} onPress={() => props.like(props.recs[props.index].id)}>


neilsarkar · 2017-11-01T01:39:23Z

app/components/RecInfoView.js

+          :
+          null
+        }
+        <ProfileView {...props} {...state} user={props.recs[props.index]} />


I totally get the appeal of passing all props and state as props to the ProfileView, but this can be hard to follow for a future dev who wants to re-use the component and wants to see what props she needs to provide to the component. You can either pick from the state explicitly eg like={props.like} topValue={state.topValue} heightValue={state.heightValue} or you can define PropTypes in the component itself (at the top of the file) so that she can look at it and see quickly what is required without parsing the code of the component jsx-eslint/eslint-plugin-react#237 (comment)

neilsarkar · 2017-11-01T01:41:38Z

coo so I'm gonna use the "requested changes" vs "commented" and we'll see if that works for showing which things are optional and which are blockers. eg this last review was optional stuff and nice-to-haves

benjaminben · 2017-11-01T04:17:00Z

So my thing is, I really don't like assigning state values from props bc it's led to buggy behavior in the past. I feel like it doesn't really make sense to keep track of the scene (and fuss with updating it) in both Stage.js and in redux. Could you give a more specific example of a type of transition that would be incompatible with this setup? Are you saying that animation might get set to different values throughout the course of one transition?

neilsarkar · 2017-11-01T05:35:00Z

Right so there are two scenes and two views. One scene is the one we were on before, let’s call that previousScene. The other scene is the one we’re on now (slash transitioning to) let’s call that the current scene. One of the views is animated, let’s call it dynamic. The other is fixed, let’s call it static.

For a fade in, we put the current scene in the static view and the previous scene in the dynamic view. Once the animation is finished, we remove the previous scene.

If we wanted a drop in (eg like when the leaderboard in egg peg drops down from the top), we’d put the current scene in the dynamic view and the previous scene in the static view. Then when the animation was finished, we’d put the current scene in the static view and remove the previous scene.

In order to achieve that using the store, the store would need to be aware of the implementation details of the view, which feels like a muddled separation of concerns.

We could possibly make it clearer by making both views dynamic and using z-index, but my spidey sense tells me it might be less performant to always have the current scene in an Animated.View, I could be wrong tho.

neilsarkar · 2017-11-01T05:50:27Z

So actually my main thing is that the caller should never have to know about the implementation details of the animation. As long as the api from a dispatch point of view is just supplying a name and transition name (and not specifying which scene goes in which container), I’d be cool with pushing the responsibilities of storing state transitions into redux (although I still feel it’s more of a view concern than a data concern) but I don’t have as strong an opinion on using shared state vs component state as I do on the caller not needing to remember implementation details of the transitions.

benjaminben · 2017-11-01T09:04:25Z

The caller (the store right?) doesn't know anything about the nature of the animations beyond their existence or not... I kinda see it like Stage.js is the production crew who handles set changes between scenes. So Stage just receives a heads up from the store, who acts as the runner/coordinator for the director (which I guess is the user's intention)? The store tells stage to `fadeIn`, but doesn't include technical deets on what fading in even means or how to do it. Also I'm not sure the `previousScene` is the one we'll always wanna have in the `Animated.View`, I can think of transitions (including swiping / sliding) where we'd want to animate both the previous and current scene over the course of the transition, likely simultaneously. Another approach I'm partial to is letting each scene handle its own entrance/exit animations, that's typically been my approach in and outside of react. That way the caller is basically just cueing actors who've memorized their marks, and the `stage` is just a stage.

neilsarkar · 2017-11-01T09:21:23Z

The caller would be whatever component you’re dispatching scene:change from.

That analogy actually makes an argument for doing the next/previous scene juggling in the component’s state, not the store. If the store sends one direction (change to scene “matches” with transition “fadein”, for eg) then the production crew should be responsible for handling switching the scenes and disposing of the previous scene, instead of the current setup where the production crew switches the scenes and then tells the director “ok now tell me to remove the previous scene”)

Point taken on using two animated views, it’ll be more flexible for swipe transitions etc.

Regarding distributing responsibility for displaying the transition to each container, I see some tradeoffs there. On the plus side, Stage.js becomes extremely simple again (probably reverts to pretty much what it was). On the minus side, adding a new scene with the same transition as an existing scene is harder — you have to remember what the scene was that had that transition, and then copy pasta the same code over to the new container. Another minus is that removing the previous scene becomes harder. Would each scene container have a conditional that says “if the current scene is not this scene and the animation is finished, render null)? The last minus is that you no longer have the benefit of being able to read the file you’re in (Stage.js) and having a full menu of transitions that have already been implemented to reuse or tweak.

I feel like the benefit of keeping Stage.js dumber is actually not that great. Since it exists and has no other responsibilities, it doesn’t hurt too much to give it the responsibility of handling transitions. Plus it provides a nice chokepoint were we to want to implement any global behavior on scene transitions (e.g. sending an analytics call that the scene has changed)

neilsarkar · 2017-11-01T09:26:51Z

Oh the last thing is that if a scene can be reached through multiple transitions (eg recs can fade in after login or swipe in from matches) that’s 0 extra lines in Stage.js and a bunch of extra lines in Recs.js.

If you feel strongly that there are benefits I’m not thinking of let me know. eggpeg and all my previous apps had each scene responsible for handling its own transition, then one day I refactored to use Stage.js and as you can see I quite like it. But my pride about how sweet this abstraction is shouldn’t stand in the way of a system that’s objectively better.

benjaminben · 2017-11-01T18:02:59Z

Okay I see the advantage of putting all the major transitions in Stage.js, however I'm not sure how to implement in a case like the previously attached .gif wherein the ChatView is rendering / transitioning over the MatchView, i.e we need both "scenes" to be rendered at all times (at least as long as that Match is open). To push the analogy further, that type of set change would be one that is conducted by the actors within the scene.

benjaminben · 2017-11-01T18:04:19Z

If we keep every scene rendered / in memory all the time then I guess it would eliminate some concerns while birthing new ones

neilsarkar · 2017-11-01T18:31:03Z

It doesn’t seem like we need both scenes to be rendered at all times. Frontend performance is all about maintaining an illusion while loading resources as lazily as possible. So in this case, here’s what we need to maintain the illusion:

When the chat transitions out, the primary profile photo should remain fullscreen behind it while the deets view transitions in
When the deets view transitions out, the primary profile photo should remain fullscreen while the chat view transitions in

We can maintain this illusion by providing a transition in Stage.js that uses a fullscreen photo in between the transition.

neilsarkar · 2017-11-01T18:31:36Z

Oops hit the wrong button it’s 3:30am

neilsarkar · 2017-11-01T18:36:49Z

eg you would add the photo (with the mask over it) to chat.js, then your dispatch calls would look like { type: ‘scene:change’, scene: { name: ‘deets’, transition: ‘slideup’, backgroundImage: ‘null.png’ } }

Don’t know if deets/slideup are accurate but hopefully u see what im saying

neilsarkar · 2017-11-01T18:48:13Z

Also it is possible that you’ll see a flicker if react native hasn’t put image caching and reuse in its core yet but dwai there’s a package that will make it so that the image is reused all three times it’s loaded (once in Chat, once in Deets, once in Stage as the background layer)

And you’ll probably have to set some prop on Chat/Deets either via mapStateToProps or delegated from Stage so that those scenes know not to render the image in the background until the transition is done

neilsarkar · 2017-11-01T18:54:03Z

Also if this is all too much to deal with we can punt on it and implement the illusion later. It’s probably kind of a lot with everything else this week

benjaminben · 2017-11-01T19:07:33Z

It just seems like so much extra - the profile is already using a FlatList so it's not rendering anything that isn't on screen anyway, it injects so much complexity to do this whole dance of swapping in a placeholder photo at the same index as the one the user was looking at when they return to the conversation, then preserving that index to feed it back to the Profile view so the user can return to the same position they were at in the match's profile. That feels way more costly just letting the scene manage itself, even if it's less abstracted.

benjaminben · 2017-11-01T19:13:29Z

This PR is getting p deep without a merge and I'm wondering if I should branch off of my the bb.style for the user --> store stuff since I need pieces of this branch (e.g the MatchView to test)?

neilsarkar · 2017-11-01T19:14:51Z

Ya that’s a good point about needing to know which index of the photo they were on, didn’t realize it did that. It’s fine for now, let’s punt

neilsarkar · 2017-11-01T19:15:53Z

Is it stable now? We can merge it if it is

neilsarkar · 2017-11-01T19:20:42Z

I ask bc the last commit had “wip” As soon as it’s stable u can hit that merge button. I’m gonna go to bed

benjaminben · 2017-11-01T19:22:08Z

yea i mean everything functions as is, i'll remove the testing fadeIn transition from the matches scene shift and merge when i get home. thanks for the early hour ping pong :D

benjaminben added 4 commits October 26, 2017 17:25

super rough flow between chat and match profile

bd80114

improve back button functionality

dea3f8e

add BlurView because this is a premium product.

6cac0ea

tweak

3755b8a

neilsarkar self-requested a review October 31, 2017 05:40

neilsarkar requested changes Oct 31, 2017

View reviewed changes

extract <LinearGradient /> to ProfileView.js; service out Dimensions

baac131

neilsarkar reviewed Nov 1, 2017

View reviewed changes

wip: migrate animation responsibility from views to store+stage

c19ba31

neilsarkar closed this Nov 1, 2017

neilsarkar reopened this Nov 1, 2017

remove temp fadeIn from Header.js matches dispatch

4a61490

benjaminben merged commit 8e812ec into master Nov 1, 2017

Rough match / chat #31

Rough match / chat #31

Conversation

benjaminben commented Oct 27, 2017

benjaminben commented Oct 27, 2017

neilsarkar commented Oct 31, 2017 • edited

neilsarkar 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

neilsarkar commented Oct 31, 2017 via email

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

neilsarkar commented Nov 1, 2017

benjaminben commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

benjaminben commented Nov 1, 2017 via email • edited

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

benjaminben commented Nov 1, 2017

benjaminben commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

benjaminben commented Nov 1, 2017

benjaminben commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

neilsarkar commented Nov 1, 2017

benjaminben commented Nov 1, 2017

neilsarkar commented Oct 31, 2017 •

edited

benjaminben commented Nov 1, 2017 via email •

edited