Adding more component jest snapshots

[marcelomorgado]

Refs #22

[pcowgill]

I'm reviewing just the last commit in this PR since the bulk of the changes have already been reviewed in #29 - this is a branch off of that feature branch.

@marcelomorgado You added snapshots for screens but not components in this PR, right? Are there any presentational components we don't have snapshots for you?

[pcowgill]

@marcelomorgado I think for any screens that also happen to be presentational (that don't have any internal state or lifecycle hooks), we probably don't need to import NavigationTestUtils and use resetInternalState, right?

On that topic, would it be easier to keep track of which screens are presentational if we moved most of their content to a presentational component and then made the "screen" just render that presentational component? I'm inclined to say yes.

[marcelomorgado]

Right.

Yes, make sense.
Some questions:

1. Make sense use Screen suffix on file names to avoid name duplication (ex.: screen/ListLandsScreen.js and components/presentational/ListLands.js)?
2. Is to do that with all screen components?

[pcowgill]

1. I like that suffix idea.
2. Yeah, I think we might as well do this with all screen components.

Thanks!

[marcelomorgado]

Hey @pcowgill what do you think?

EthereumQuestionScreen

import React from "react";
import EthereumQuestion from "../components/presentational/EthereumQuestion";

export default class EthereumQuestionScreen extends React.Component {
  render() {
    return (
      <EthereumQuestion
        onSignUp={() => this.props.navigation.navigate("EthereumSignUp")}
        onSignIn={() => this.props.navigation.navigate("EthereumSignIn")}
      />
    );
  }
}

EthereumQuestion

import React from "react";
import { Button, StyleSheet, View } from "react-native";
import LargeText from "./LargeText";
import Colors from "../../constants/Colors";

export default function EthereumQuestion(props) {
  return (
    <View style={styles.container}>
      <LargeText>{`Are you new to Ethereum?`}</LargeText>
      <View style={styles.buttonView}>
        <Button title="Yep" onPress={props.onSignUp} />
      </View>
      <View style={styles.buttonView}>
        <Button title="Nope" onPress={props.onSignIn} />
      </View>
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    backgroundColor: Colors.backgroundColor,
    alignItems: "center",
    justifyContent: "center",
  },
  buttonView: {
    flexDirection: "row",
    marginTop: 20,
  },
});

[pcowgill]

This looks great!

[pcowgill]

Feel free to make changes like this one throughout the repo

[pcowgill]

Most pure, functional presentational components won't need to useFakeTimers, right?

[marcelomorgado]

Yes, you are right.

[pcowgill]

I think the snapshot / test split would be more readable if there were a presentational component for most of the jsx in LandClaim that had a shallow snapshot test with no navigation mock, and then this screen shallow-rendered itself with the name of that component like it does now, but the snapshot would be much smaller.

[marcelomorgado]

Yes, I agree. 👍

[marcelomorgado]

I'm reviewing just the last commit in this PR since the bulk of the changes have already been reviewed in #29 - this is a branch off of that feature branch.

@marcelomorgado You added snapshots for screens but not components in this PR, right? Are there any presentational components we don't have snapshots for you?

Sorry, I've read literally the title of the issue: "Add jest snapshots for the existing screens", I think that all components should be tested.

[pcowgill]

Ah yeah, I should have mentioned components in the issue too. That said, it's a perfectly sensible choice to add snapshots for the components in another PR. But it's probably simplest given the screens/components moving parts we just discussed to do it within this PR this time around

[marcelomorgado]

Ah yeah, I should have mentioned components in the issue too. That said, it's a perfectly sensible choice to add snapshots for the components in another PR. But it's probably simplest given the screens/components moving parts we just discussed to do it within this PR this time around

Ok, I got it.

[marcelomorgado]

Some notes:

1. I'm not sure if ListLandScreen refactoring that I've made is the better one;
2. On some components, I have to keep onPress prop name because I've not found a better name;
   3, Note: EnthereumSignUpForm-> Continue (Account.create call) is delaying the react-navigation (after click, go back is very slow). Do you want that I investigate that in this PR ?;
3. Should I keep jest / react-navigation boilerplate on all screen tests?

jest.useFakeTimers();
  beforeEach(() => {
    NavigationTestUtils.resetInternalState();
  }); 

[pcowgill]

1. I'll keep an eye out for ListLandScreen, thanks
2. Fine with me
3. I noticed this too. No need to investigate that in this PR, but I'll create an issue for it.
4. (a.k.a. the second 3) I think keeping that boilerplate on all screens seems fine for now. I imagine that most screens won't have state when we introduce redux, but that change can come later. Fake timers for screens miiight be useful. I haven't decided yet.