Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add destructuring actions with props example for createReducer… #1987

Merged
merged 3 commits into from
Jul 9, 2019

Conversation

jordanpowell88
Copy link
Contributor

… (#1985)

Closes #1985

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[X] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Currently their is no documentation around how to access actions with props in the on method of createReducer

Closes #1985

What is the new behavior?

1.) ad an action with a prop
2.) show how to destructure the action to access the prop in the on method of createReducer

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 2, 2019

Preview docs changes for 6a2d79a at https://previews.ngrx.io/pr1987-6a2d79a/

@wesleygrimes
Copy link
Contributor

Thanks for your submission @jordanpowell88! We will take a look and get back to you.

Copy link
Contributor

@wesleygrimes wesleygrimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jordan,

This is looking really good. I like the example. I think we need to add some additional wording around the use of props and how they work.

What are your thoughts about beefing up the wording around the usage of the new createReducer function?

I am thinking...

Sort of like a decomposition of it:

  • What input parameters it takes in
  • Details about the on function, including how the first parameter is the action and the second parameter is a handler or projector for new state that can optionally accept a second argument for props which can be destructured for better developer ergonomics?

@jordanpowell88
Copy link
Contributor Author

I agree, I debated whether to do this in my initial commit as it seamed unclear to me. I will work on adding those changes

@wesleygrimes
Copy link
Contributor

wesleygrimes commented Jul 2, 2019

Thank you @jordanpowell88! I think getting the new reducer function documented will make this even more of a super valuable PR.

@timdeschryver
Copy link
Member

fwiw, @jtcrowson documented the new API create functions at #1980. If you click on the on function in the code snippets, you'll be redirected to these API docs. As addressed in that same PR, we have to be careful to not document everything twice, as it will be harder to maintain.

I'm not saying we don't have to document it, I just want to raise some awareness that #1980 exists 🙂

@wesleygrimes
Copy link
Contributor

I agree we don't want to duplicate efforts, but we need a place to gently guide folks into the functionality.

So, how about we use the API documentation from @jtcrowson as a baseline and pull out the highlights into this page? Then we create hard links to the API docs from this area so that we don't duplicate docs.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 2, 2019

Preview docs changes for fa30310 at https://previews.ngrx.io/pr1987-fa30310/

@jtcrowson
Copy link
Contributor

Agree that there is some duplication between here and #1980.

Using Angular.io as inspiration, API-level docs explain the function and it's parameters, while general docs explain how to use the function in combination with others to accomplish application functionality.

For this case, I like that the general docs already show defining a set of actions, a state, an initial state, then using them in a reducer. But I'm not sure we need a createReducer section to explain the function in isolation.

I'd advocate adding the additional example to the existing section and dropping the createReducer section in favor of #1980:

const scoreboardReducer = createReducer(
  initialState,
  on(ScoreboardPageActions.homeScore, state => ({ ...state, home: state.home + 1 })),
  on(ScoreboardPageActions.awayScore, state => ({ ...state, away: state.away + 1 })),
  on(ScoreboardPageActions.resetScore, state => ({ home: 0, away: 0 })),
  on(ScoreboardPageActions.setScores, (state, { game }) => ({ home: game.home, away: game.away }))
);

@wesleygrimes
Copy link
Contributor

I am also in agreement that we don’t want duplication. We do, however, want this part of the guide/website to gently guide folks to the official API docs section on createReducer. As I stated earlier, we need to introduce the concept of the new style reducer here, add high level details on using the on method with props and show a good example of destructuring. Then in that high level example put a link to point folks directly to the more detailed API docs on createReducer.

@jordanpowell88
Copy link
Contributor Author

Do you feel my most recent commit accomplish this @wesleygrimes ?

@wesleygrimes
Copy link
Contributor

wesleygrimes commented Jul 3, 2019 via email

@brandonroberts
Copy link
Member

#1980 has landed so this one can be revisited to see if these changes are still needed.

@brandonroberts brandonroberts added the Needs Cleanup Review changes needed label Jul 7, 2019
@wesleygrimes
Copy link
Contributor

OK, nice work @jtcrowson on #1980. I agree we don't need an entire section here. @jordanpowell88 let's simplify this PR and just add the example with destructuring. There is already a link to the API section for createReducer so that covers getting folks to the detailed API docs.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Jul 9, 2019

Preview docs changes for feb8d0d at https://previews.ngrx.io/pr1987-feb8d0d/

Copy link
Contributor

@jtcrowson jtcrowson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wesleygrimes wesleygrimes removed the Needs Cleanup Review changes needed label Jul 9, 2019
Copy link
Contributor

@wesleygrimes wesleygrimes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good to merge.

@wesleygrimes wesleygrimes merged commit e377005 into ngrx:master Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Add example to createReducer of destructuring actions with props
6 participants