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

Document ReduxInjector #14

Merged
merged 5 commits into from
Nov 14, 2017
Merged

Conversation

maier49
Copy link
Contributor

@maier49 maier49 commented Nov 7, 2017

The following has been addressed in the PR:

  • There is a related issue
  • All code matches the style guide
  • Unit or Functional tests are included in the PR

Description:
Adds the redux injector documentation from the PR to the README

Resolves #9

@maier49 maier49 requested a review from kitsonk November 7, 2017 17:16
@dylans dylans added this to the 2017.11 milestone Nov 7, 2017
@agubler
Copy link
Member

agubler commented Nov 7, 2017

@maier49 the example on the PR is now slightly out of date, I'll comment on the diffs in the morning :)

@agubler
Copy link
Member

agubler commented Nov 7, 2017

This is a real world usage that is up to date https://github.com/dojo/examples/blob/master/todo-mvc-tsx/src/main.ts

@maier49
Copy link
Contributor Author

maier49 commented Nov 8, 2017

@agubler I updated the example, and also the wording, based on that example. Let me know if the updated wording needs any tweaks.

Copy link
Member

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

I don't know enough about the ReduxInjector, so it looks good from my point of view but defer to @agubler

README.md Outdated
*Coming Soon*
`ReduxInjector` can be used to bind a redux store and Dojo 2 widgets using the `registry`.

An injector can be defined in the registry, which is then registered with the `Router`, and provided to the `Projector` as one of its properties. This is demonstrated in the example below.
Copy link
Member

@agubler agubler Nov 8, 2017

Choose a reason for hiding this comment

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

I don't understand this sentence, the Router doesn't have anything to do with the using the ReduxInjector...

README.md Outdated
const store = createStore(todoReducer, defaultState, global.__REDUX_DEVTOOLS_EXTENSION__ && global.__REDUX_DEVTOOLS_EXTENSION__());
registry.defineInjector('application-state', new ReduxInjector(store));

const config = [
Copy link
Member

Choose a reason for hiding this comment

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

Can drop anything router related from the examples (might be confusing)

README.md Outdated
};

const registry = new Registry();
const store = createStore(todoReducer, defaultState, global.__REDUX_DEVTOOLS_EXTENSION__ && global.__REDUX_DEVTOOLS_EXTENSION__());
Copy link
Member

@agubler agubler Nov 8, 2017

Choose a reason for hiding this comment

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

might want to call out that this initialises the redux dev tools extension (and maybe a link to it?).

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this in the readme?

Copy link
Member

Choose a reason for hiding this comment

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

Nope probably not to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the redux dev tools extension from the example code and removed the explanation about it.

@maier49
Copy link
Contributor Author

maier49 commented Nov 8, 2017

@agubler Updated again

@maier49 maier49 merged commit 713c7f0 into dojo:master Nov 14, 2017
@maier49 maier49 deleted the document-redux-injector branch November 14, 2017 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants