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

Update Redux setup #21

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Update Redux setup #21

wants to merge 5 commits into from

Conversation

lyndsherb
Copy link

@lyndsherb lyndsherb commented Aug 24, 2018

Updates the React/Redux integration to be more streamlined.

Previously, the generator was calling this.context.store.getState() or this.context.store.dispatch() in a lot of places to trigger the actions. This is repetitive, boring, and not exactly best practice. This StackOverflow answer explains it like so:

{...} Primarily because it really muddies the line between container and presentational components. Presentational components don't need access to store, and there are other methods to do this which don't have this (albeit pedantic) drawback.

react-redux, however, provides us with a handy connect function which allows us to hook the Redux state and the actions into the props of our application. This allows us to pass the application state down as necessary.

The presentational components have also been updated to be function components rather than Class components. Functional components are simpler than Class components, and as such are a better fit when all we want to to is render out JSX. More information can be found in this article by Dan Abramov.

The setup itself was helped along with this article from Code Like A Girl.

I also updated the tests to provide more thorough examples, such as testing the React output and the return on both actions and reducers.

Todo

  • Add babel-plugin-transform-object-assign and redux-mock-store to Magepack SDK; one for browser compatibility, one for better testing.
  • Ensure this can be easily integrated into Magento as expected
  • Ensure the store can be accessed outside the app

@lyndsherb lyndsherb changed the title [WIP] Feature/update redux setup Update Redux setup Aug 29, 2018
Copy link

@convenient convenient left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with this redux stuff but your changes and reasoning do look sensible.

I'd ask that you try and get a review from torrey and nick, perhaps chase up with a PM if you need their time.

const reducer = {
[Types.SELECT_ITEM]: () => ({
...state,
items: state.items.map(function(item) {

Choose a reason for hiding this comment

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

Could still use arrow funcs here? If only for consistency

return ({
...state,
selected: false
});

Choose a reason for hiding this comment

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

Could just do:

return ({
  ...state,
  selected: state.id === action.id
});

It's not really clearer tho tbf

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.

3 participants