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

[WIP] Swap alt with redux #59

Merged
merged 15 commits into from
Aug 27, 2015
Merged

[WIP] Swap alt with redux #59

merged 15 commits into from
Aug 27, 2015

Conversation

justin808
Copy link
Member

Moving from alt to redux, with immutable.js.

Pending Issues

  • Clear out input form after submit.
  • Proper functioning of processing indicator.

@justin808
Copy link
Member Author

A few observations:

  1. It's a bit of work to use the immutable.js API. You have to be very careful about introducing mutable objects inside of it.
  2. Going from multiple reducers to "data" is somewhat confusing: https://github.com/justin808/react-webpack-rails-tutorial/pull/59/files#diff-46faf85c75e5d9204659168ddf8e718dR1. I'm not sure what to name these parts.

Thanks @alexfedoseev for the help!

@justin808
Copy link
Member Author

Getting there. We've got a few bugs, and we're going to make this work with server side rendering, @alexfedoseev

justin808 and others added 6 commits August 22, 2015 11:52
This is before changing to use immutable.js
Next issue is:

Uncaught Error: Invariant Violation: The return value of `select` prop
must be an object. Instead received Map { "comments": , "ajaxCounter":
0, "fetchCommentError": "", "submitCommentError": "" }.
Squash merge of
#60 from
@alexfedoseev (thanks!)

1. What should we name the reducer? Seems confusing in terms of how the
reducer name gets put as part of the state.
2. Functional issue in that we don't clear out the last name/commment
after ajax is successful. Seems that we would need to have the component
state inside of the main store.
3. Ajax counter to show busy indicator of async in progress might not be
working.
4. We should show a simple validation on the server and the display of a
simple error message on the form.
* For the phantomjs issue: ariya/phantomjs#12401
* Still getting a warning
  iterable.length has been deprecated, use iterable.size or iterable.count(). This warning will become a silent error in a future version. Error:
    at :4602
@justin808
Copy link
Member Author

NOTE: I rebased this branch on top of master to have a clean history of changes.
cursor

@justin808
Copy link
Member Author

cursor

cursor

cursor

@justin808
Copy link
Member Author

@mapreal19 and I are pairing on this on his branch off of my branch. We decided to have me rebase this one on top of master to have a linear history. So I did rebase origin/master and then push -f.

@mapreal19 had to do essentially these steps:

http://stackoverflow.com/a/18909381/1009332

And then we cherry-picked his change on top of that.

Then we ran into an issue with poltergeist not working on his machine.

Stay tuned. We're going to pick this up tomorrow.

@justin808
Copy link
Member Author

@mapreal19 Pending:

  • Show server side errors
  • Fix tests

Future Enhancements:

mapreal19 and others added 3 commits August 25, 2015 01:17
- Added gem "phantomjs" (useful if not installed locally)

- Fixed test suite and refactored
…-redux

* mapreal19-swap-alt-with-redux-mario:
  Fixed some issues:
  resetting the form after submit should reset and focus text not author
  add error reporting to form and ensure form is reset on succesful save
  fix ajax loading message
* Still some js linting failures
* Run
  rake lint
  to see them
@justin808
Copy link
Member Author

@mapreal19 @PlasticLizard Looks great! Any volunteer to fix the linting issues? Incidentally, this linting setup is exactly what we use on my other projects.

mapreal19 and others added 3 commits August 26, 2015 11:30
- Updated eslint file
…with-redux

* mapreal19-swap-alt-with-redux-fix-lints:
  Fix lint errors
justin808 added a commit that referenced this pull request Aug 27, 2015
@justin808 justin808 merged commit 6917469 into master Aug 27, 2015
@justin808 justin808 deleted the swap-alt-with-redux branch August 27, 2015 22:06
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.

None yet

4 participants