PLANNER-1339: Add Redux as state container #23
Conversation
|
The rest of the build errors (apart the one I commented on inline) can be fixed by running |
|
Every source file (.js, .css, .java, etc.) must have a license header (I think most of configuration files don't have to, so for example Action item for me: set up an automated check for license headers that will fail CI build if it's missing. |
danielefiungo
changed the title
| let webSocket; | ||
| let stompClient; | ||
|
|
||
| function mapEventToActions(dispatch) { |
There was a problem hiding this comment.
@danielefiungo Is there a good reason to name the function like this? I'm a bit confused. I would expect it to be called perhaps subscribe() or subscribeToRouteTopic() or similar.
There was a problem hiding this comment.
@danielefiungo the code looks very nice. However, the refactoring introduces two regressions:
- Automatic re-connection doesn't work.
- UI doesn't work correctly when there is 1 or 2 locations.
- If I click an empty map, it remains empty although the home location is added on backend. It's just not displayed until I add the second locations.
- When there are 2 locations (home + 1 visit) and I remove the visit location, it is not removed from the map.
Under normal circumstances, we would need to fix those bugs before the PR can be merged. At this stage, I'm willing to merge if it help your workflow (let's talk about this on IRC).
From test coverage perspective, it would be really nice if you could write a test that fails due to the UI bug before the fix is made. Since we currently don't have a test that would detect the bug, there's obviously missing test coverage (can we detect it?). If we fix the bug without adding test first, the coverage won't improve and the same bug can be introduced again.
|
Reconnection now works, thanks! |
yurloc
changed the title
No description provided.