Rough match / chat

[benjaminben]

[benjaminben]

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]

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: https://github.com/superseriouscompany/mayte/pull/31#discussion_r147931800

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.

1. It will need to be aware of the scene to be shown
2. It will need to be aware of the previous scene that was being shown
3. 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:

1. We'll need a containing View around the next scene and a containing Animated.View around the previous scene
2. 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]

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 notifications@github.com wrote:

@benjaminben commented on this pull request.

In app/containers/Header.js https://github.com/superseriouscompany/mayte/pull/31#discussion_r148077253 :

• },
• 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 https://github.com/superseriouscompany/mayte/pull/31#discussion_r148077253, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFEpZSiRBANjz6JqAidrI0FIq8SQXUeks5sx17OgaJpZM4QIbPy .

-- Neil

[neilsarkar]

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]

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]

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]

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]

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]

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]

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]

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]

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

[neilsarkar]

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:

1. When the chat transitions out, the primary profile photo should remain fullscreen behind it while the deets view transitions in
2. 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]

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

[neilsarkar]

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]

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]

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]

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]

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]

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]

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

[neilsarkar]

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]

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