-
Notifications
You must be signed in to change notification settings - Fork 490
Updating embark demo to use react instead of jquery #382
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
Conversation
Blockchain tab complete Whisper tab complete (requires status indicator yet)
jrainville
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small requests. Otherwise, this is pretty dope! 👍
|
|
||
| this.setValue = this.setValue.bind(this); | ||
| this.getValue = this.getValue.bind(this); | ||
| this.handleChange = this.handleChange.bind(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an FYI, you don't need to premptively bind your functions if you call them with arrows. Eg:
onClick={(e) => this.setValue(e)}
or
handleChange = (e) => {
this.setState({valueSet: e.target.value});
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the components based on this suggestion. Also, there were some let _this = this; that were removed after changing the functions to arrow functions.
| type="text" | ||
| defaultValue={this.state.valueSet} | ||
| onChange={this.handleChange}/> | ||
| {' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? If it is just to add a spacer, maybe use a css margin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: Added a margin-right to input.form-control in dapp.css to avoid having to use spacers
| }) | ||
| .catch(function(err) { | ||
| if(err){ | ||
| console.log("Storage uploadFile Error => " + err.message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should show the error with a state change for it to be clearer to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: The screen shows an error notice when there's an error when using the storage.
| defaultValue={this.state.listenTo} | ||
| placeholder="channel" | ||
| onChange={e => this.handleChange(e, 'listenTo')} /> | ||
| {' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, use a margin if this is only for space.
templates/demo/app/dapp.js
Outdated
| componentDidMount(){ | ||
| let _this = this; | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a timeout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmbarkJS.Messages.Providers.whisper is not available when the component mounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iurimatias , would there be a better way to wait for whisper, like an event?
jrainville
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only forgot one spacer. Also, I'm wondering if Iuri has an answer the timer needed for Whisper.
| type="text" | ||
| defaultValue={this.state.valueSet} | ||
| onChange={(e) => this.handleChange(e)} /> | ||
| {' '} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot one here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! fixed now!
|
@jrainville Removed the timeout by using |
|
One of test fails |
Adding swarm to embarkjs. WIP. Add 'auto' setting for geth CORS and websockets origin * 'auto' now supported for `rpcCorsDomain` and `wsOrigins` in the blockchain config. * 'auto' set to the default value in blockchain config for test and demo apps. test add config and contract and add test addFileToPipeline test and registerBeforeDeploy with new arg add more registers but generation one fails in run WIP commit Undo changes to test config. Merge pull request #381 from embark-framework/features/cors-auto Add 'auto' setting for geth CORS and websockets origin fix a bug where upload cmd used plugin name don't error if it's an empty dapp with no contracts yet Merge pull request #383 from embark-framework/no_contracts don't error if it's an empty dapp with no contracts yet remove duplicated entry force zepplein version for travis Merge pull request #384 from embark-framework/chores/test-allpligin-apis Small fixes for plugin APIs intercept logs in the app itself - stopgap fix Merge pull request #385 from embark-framework/console_logs_fix intercept logs in the app itself - stopgap fix * removed unneeded provider property. * add 'swarm' as a provider in the storage.config * update method for swarm service check Merge branch 'develop' into features/add-swarm-to-embarkjs More work to add swarm to embarkjs * added eth-lib to parse result of swarm text * changed "currentStorage" and "currentMessages" to "currentProvider" for consistency. * added protocol to storage config * selectively starts storage service depending on which one is configured in the storage config * run service check for ipfs/swarm prior to uploaded * added swarm methods for embarkjs Updated code based on code review check if testrpc is installed and warn if not Merge pull request #386 from embark-framework/bug_fix/test-rpc-not-installed check if testrpc is installed and warn if not Removed timeout Removed spacer Merge pull request #382 from embark-framework/react-demo Updating embark demo to use react instead of jquery fix on contract add Merge pull request #387 from embark-framework/bug_fix/new-contract-in-empty-dapp Fix adding a contract redeploy with right config on config change fix tests reset watchers after build to make sure files remain watch Merge pull request #389 from embark-framework/bug_fix/file-changes-not-watched Fix files not being watched Merge pull request #388 from embark-framework/bug_fix/changing-contract-config Redeploy with right config on config change Added swarm support in embarkjs and isAvailable for messages/storage * reverted currentProvider back to currentStorage and currentMessages * added `EmbarkJS.Storage.isAvailable` and `EmbarkJS.Messages.isAvailable()` and underlying provider functions for Whisper, Orbit, IPFS, and Swarm * Finished swarm implementation in embarkjs plus cleanup * updated test app storage config to swarm to show swarm config option Merge branch 'develop' into features/add-swarm-to-embarkjs
| this.state = { | ||
| whisperEnabled: false, | ||
| storageEnabled: false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #390 (not yet approved/merged), this can be achieved with EmbarkJS.Storage.isAvailable() and EmbarkJS.Messages.isAvailable()
No description provided.