Addon groups UI #13
Addon groups UI #13
Conversation
| @@ -14,3 +14,4 @@ rules: | |||
| no-use-before-define: [warn, { functions: false }] | |||
| react/no-multi-comp: [off] | |||
| no-throw-literal: [off] | |||
| import/no-unresolved: [warn] | |||
There was a problem hiding this comment.
This is included in airbnb as [error], right? I'm curious why you changed it?
There was a problem hiding this comment.
i was getting errors with:
https://github.com/mozilla/morgoth/pull/13/files#diff-535db65efe188b02b6f9792222d76a0bR4
Unable to resolve path to module 'redux-form-material-ui'.
couldn't get rid of them and it was working as intended so i switched to a warning so i could commit. but maybe there's a way of fixing the error?
There was a problem hiding this comment.
I find this rule really useful, so I think it would be good to not lower it. I'll poke at this and see if I can get it working.
There was a problem hiding this comment.
I can reproduce the problem here. Maybe try removing node_modules, and reinstalling based on #15?
There was a problem hiding this comment.
I'm still seeing the issue. Did a complete reinstall after rebasing and pinning the new dependencies. Then I tried upgrading ESLint. Ultimately committed with a --no-verify to see what Circle thinks.
| function receivedUpdateAddonGroup(addonGroup, saveAndContinue) { | ||
| if (!saveAndContinue) { | ||
| browserHistory.push('/addon_groups/'); | ||
| } |
There was a problem hiding this comment.
Action creators should be pure, that is, have no side effects. I think it would be better to use react-router-redux's action creators for changing browserHistory, described here. This lets the redux devtools work correctly. We've found those very helpful on Normandy.
|
My eyes sort of glazed over, but I don't see any other issues |
| dispatch(push(`/addon_groups/${addonGroup.id}/`)); | ||
| } else { | ||
| dispatch(push('/addon_groups/')); | ||
| } |
|
Circle ain't happy with this either: |
|
After much painful digging I was able to figure out the issue... |
|
Wow, that's intense. Nice job finding it. I wonder why it worked fine for me? If you want to go ahead and keep the eslint change to a warning until that PR merges, go for it. I don't have any other comments about this, for better or worse. r+ |
r?