Goal selector

[tyasw]

This change is 

[Micah-Schiewe]

Reviewed 29 of 29 files at r1, 19 of 19 files at r2.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @tyasw)

---

src/components/App/App.tsx, line 11 at r1 (raw file):

  return (
    <div className="App">
      <GoalView />

Wasn't App just supposed to be an example of how to do things, not a part of the project we modify?

---

src/components/GoalView/GoalView.tsx, line 18 at r1 (raw file):

export class GoalView extends React.Component<GoalViewProps, GoalViewState> {
  constructor(props: GoalViewProps) {

The GoalView is incompatible with localization/i18n

---

src/components/GoalView/GoalViewReducers.tsx, line 17 at r1 (raw file):

    case ADD_GOAL:
      let newHistory = addGoalToHistory(state, action.payload);
      let newSuggestions = removeGoalFromSuggestions(state, action.payload);

Are you certain that these don't mutate the arguments of the original state? (I know that Typescript's object handling is weird)

---

src/components/GoalView/GoalViewReducers.tsx, line 20 at r2 (raw file):

      return {
        history: newHistory.makeCopy(),
        goalOptions: state.goalOptions,

Much more descriptive name!

---

src/components/GoalView/TempDefaultState.ts, line 13 at r1 (raw file):

let allTheGoals: Goals[] = [];
let goal1: Goals = new TempGoal(tempUser);

Since TempGoal was removed, we may need to modify this code (same issue on lines 18, 23, 31, and 34)

---

src/components/GoalView/GoalComponent/GoalComponent.tsx, line 10 at r1 (raw file):

export class Goal extends React.Component<GoalProps> {
  constructor(props: GoalProps) {

I'm not sure if it matters, but this Goal class isn't compatible with localization/i18n (see last comment before modifying)

---

src/components/GoalView/GoalComponent/GoalComponent.tsx, line 21 at r1 (raw file):

    );
  }
}

In Sam and my edits, we had basically added a new class with this functionality called BaseGoalSelect; we'll need to look into merging the two.

---

src/components/GoalView/GoalFuture/GoalFutureComponent.tsx, line 11 at r1 (raw file):

  all: Goals[];
  suggestions: Stack<Goals>;
}

Why does this class's props include the history and all possible goals, if it is only utilizing the suggestions?

---

src/components/GoalView/GoalFuture/GoalFutureComponent.tsx, line 14 at r1 (raw file):

export class GoalFuture extends React.Component<GoalFutureProps> {
  constructor(props: GoalFutureProps) {

GoalFuture is incompatible with localization/i18n

---

src/components/GoalView/GoalHistory/GoalHistoryComponent.tsx, line 11 at r1 (raw file):

  all: Goals[];
  suggestions: Stack<Goals>;
}

Like on GoalFutureComponent, why is the history aware of the present and future? It just seems a bit unnecessary to me.

---

src/components/GoalView/GoalHistory/GoalHistoryComponent.tsx, line 13 at r1 (raw file):

}

export class GoalHistory extends React.Component<GoalsState> {

Th GoalHistory is incompatible with localization/i18n

---

src/components/GoalView/GoalSelector/GoalSelectorComponent.tsx, line 12 at r1 (raw file):

  all: Goals[];
  suggestions: Stack<Goals>;
}

I can understand why the present component needs access to the future (to set it's initial value to what we want to suggest), but why does it know about the history?

---

src/components/GoalView/GoalSelector/GoalSelectorComponent.tsx, line 15 at r1 (raw file):

export class GoalSelector extends React.Component<GoalSelectorProps> {
  constructor(props: GoalSelectorProps) {

The GoalSelectorComponent isn't compatible with localization/i18n

---

src/components/GoalView/GoalSelector/GoalSelectorComponent.tsx, line 26 at r1 (raw file):

      this.props.addGoal(goal);
    }
  }

Just a thought: you might be able to put this function outside of the class in this file, and bind it from there. That would help us in our minor testing issues.

---

src/components/GoalView/GoalSelector/GoalSelectorComponent.tsx, line 34 at r1 (raw file):

      }
    }
  }

You might be able to do the same with this function.

---

src/components/GoalView/tests/GoalViewReducers.test.tsx, line 20 at r1 (raw file):

    id: 0
  };
  const goal: Goals = new TempGoal(user);

Since TempGoal got deleted, this will need some minor modification. This also applies to multiple other locations in the file.

---

src/components/GoalView/tests/GoalViewReducers.test.tsx, line 67 at r1 (raw file):

    all: [],
    suggestions: new Stack<Goals>([])
  };

Since you're repeating this series of steps to create a default goal, maybe you should create a defaultGoal which you can access and copy at any time? It'll make this test more readable.

---

src/types/goals.tsx, line 36 at r1 (raw file):

}

export interface Goals {

In later revisions, this was renamed to Goal instead of Goals. Just FYI.

---

src/types/goals.tsx, line 31 at r2 (raw file):

  suggestions: Stack<Goal>;
}

Both interfaces have identically-named properties. That intentional?

[SamDelaney]

Reviewed 29 of 29 files at r1, 19 of 19 files at r2.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @Micah-Schiewe and @tyasw)

---

src/components/App/App.tsx, line 11 at r1 (raw file):

Previously, Micah-Schiewe wrote…

Wasn't App just supposed to be an example of how to do things, not a part of the project we modify?

No, this is fine for now.

---

src/components/GoalView/GoalView.tsx, line 13 at r1 (raw file):

export interface GoalViewState {
  goalHistory: Stack<Goals>;
  all: Goals[];

maybe a more descriptive name than 'all'?

---

src/components/GoalView/GoalView.tsx, line 31 at r1 (raw file):

      <div className="GoalView">
        <GoalHistory />
        <GoalSelector />

I think some renaming might be in the future. GoalSelector seems like a better name for what you're calling GoalView.

---

src/components/GoalView/GoalViewReducers.tsx, line 34 at r1 (raw file):

export function removeGoalFromSuggestions(
  state: GoalsState,

Not a fan of the GoalsState name, but its not the worst. I don't presently have a better idea.

---

src/components/GoalView/TempDefaultState.ts, line 38 at r1 (raw file):

Quoted 27 lines of code…

let allTheGoals: Goals[] = [];
let goal1: Goals = new TempGoal(tempUser);
let goal1Message = "A goal";
goal1.id = 1;
goal1.name = "Handle duplicates";
goal1.data = { words: goal1Message.split(" "), step: 1 };
let goal2: Goals = new TempGoal(tempUser);
let goal2Message = "Another goal";
goal2.id = 2;
goal2.name = "Handle flags";
goal2.data = { words: goal2Message.split(" "), step: 2 };
let goal3: Goals = new TempGoal(tempUser);
goal3.name = "Grammar check";
goal3.id = 3;
allTheGoals.push(goal1);
allTheGoals.push(goal2);
allTheGoals.push(goal3);

let suggestionsArray: Goals[] = [];
let suggestion1: Goals = new TempGoal(tempUser);
suggestion1.name = "Handle duplicates";
suggestion1.id = 4;
let suggestion2: Goals = new TempGoal(tempUser);
suggestion2.name = "Grammar check";
suggestion2.id = 5;
suggestionsArray.push(suggestion1);
suggestionsArray.push(suggestion2);

I get that this is a temporary thing, but this should still be easier to read, so the rest of us can deal with it if we need to.

---

src/components/GoalView/GoalComponent/GoalComponent.tsx, line 21 at r1 (raw file):

Previously, Micah-Schiewe wrote…

In Sam and my edits, we had basically added a new class with this functionality called BaseGoalSelect; we'll need to look into merging the two.

Yeah, this will be removed soon. I believe William was aware of that.

---

src/components/GoalView/GoalFuture/GoalFutureComponent.tsx, line 21 at r1 (raw file):

    return (
      <div className="GoalPicker">
        {this.props.suggestions.stack.map(goal => (

Beautifully Simple

---

src/components/GoalView/GoalSelector/GoalSelectorDropdown/GoalSelectorDropdown.tsx, line 10 at r1 (raw file):

  possibleGoals: Goals[];
  handleChange: (
    event: React.ChangeEvent<{ name?: string; value: unknown }>

Cool way of doing this. Nice!

---

src/components/GoalView/tests/GoalViewReducers.test.tsx, line 71 at r1 (raw file):

});

it("Should add goal to history", () => {

This doesn't handle any actions. Is there a better place to put these tests?

[SamDelaney]

Reviewed 19 of 21 files at r3.
Reviewable status: 28 of 29 files reviewed, 24 unresolved discussions (waiting on @Micah-Schiewe and @tyasw)

---

src/components/App/App.test.tsx, line 14 at r3 (raw file):

    goalsState: {
      history: {
        history: defaultState.history.history

Can you rename the first history to something like historyState? This looks like a mistake.

---

src/components/App/App.test.tsx, line 18 at r3 (raw file):

      goalOptions: defaultState.goalOptions,
      suggestions: {
        suggestions: defaultState.suggestions.suggestions

Same as above, with suggestions ^

[SamDelaney]

Reviewed 15 of 15 files at r4.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @Micah-Schiewe and @tyasw)

[Micah-Schiewe]

Reviewed 1 of 21 files at r3.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @tyasw)