Make it a spa! #134
Make it a spa! #134
Conversation
mattnibs
force-pushed
the
client-dir
branch
2 times, most recently
from
c4c2acc
to
ec5576d
Compare
|
screenshot: |
| let rendezvous; | ||
|
|
||
| function bundleReceived(bundle) { | ||
| api.describe(bundle) |
There was a problem hiding this comment.
Should return this promise and return bundleReceived(res.bundle) on line 23 so that the catch on line 24 will catch errors from this.
|
|
||
| export function newBundle(bundle, inputs) { | ||
| return { | ||
| type: NEW_BUNDLE, |
There was a problem hiding this comment.
Thoughts on following FSA (i.e. sticking stuff in payload)? I started following it in some repo somewhere. Not saying we have to switch to it but just wanted to bring it up in case you think its a good idea/haven't seen it.
There was a problem hiding this comment.
-1, as @aswan would say "a foolish consistency is the hobgoblin of small minds".
There was a problem hiding this comment.
Its a really good idea, I'll switch to it future work. Not going to do it in this pr.
mattnibs
force-pushed
the
client-dir
branch
4 times, most recently
from
7423aa1
to
c042895
Compare
| @@ -15,10 +15,14 @@ class JuttleViewer extends Component { | |||
| } | |||
|
|
|||
| render() { | |||
| let juttleSource = false; | |||
There was a problem hiding this comment.
juttleSource seemed like a better variable name than the generic "bundle", can we keep it?
There was a problem hiding this comment.
I think changing for consistency sake was a good idea (the thing is called a bundle everywhere else). As for changing bundle to juttleSource, thats a debate for another place
|
What's the plan for testing things like the directory listing functionality? |
|
Well I'm not going to handle it here because of the move to workbench... but, for server side we already have unit tests. For client side I'll have to mock the server and use jsdom to listen for path changes (if this is possible) |
|
Just split the commits up to isolate changes in src for the move to |
|
This seems like a big addition of new code, 500+ lines. If the tests are not coming with this PR, please file an issue that outlines which testcases should be added, and that the plan is to do it with jsdom, after the switch to workbench repo. |
| }); | ||
| } | ||
|
|
||
| export let getDirectories = (path) => { |
There was a problem hiding this comment.
this function returns a list of directories and juttle files in a given path? maybe something like getDirectory?
Lots of stuff going, here are the main features: - Moved the /run app to index - Added directory listing functionality - Implemented React-Router: changing urls no longer cause the entire app to refresh Some of the under-the-hood changes: - Restructured run app to use redux - Add font-awesome to app - Standardized error handler for api
|
looks good overall |
Lots of stuff going, here are the main features:
to refresh
Some of the under-the-hood changes: