Allow for send invoke calls from websites the user visits #27
Conversation
|
WIP, resolving merge conflicts ... I will likely stop reusing the form and just make a text only version for this case. |
|
Merging done To see the popup, run this in console on any page (compatible with previous versions of NeoLink): |
src/app/components/Header/index.js
Outdated
| @@ -6,13 +6,17 @@ import style from './Header.css' | |||
|
|
|||
| export default class Header extends Component { | |||
| render() { | |||
| const { noMenu } = this.props | |||
There was a problem hiding this comment.
This code might more clear if it was written in a more declarative way. Something like: const { hasMenu = false } = this.props. This way, we can be more truthy, and we don't need to add configuration to take away a thing we don't need, we only have to add add configuration for a thing we want.
There was a problem hiding this comment.
Good call. I'll update it.
src/app/components/Header/index.js
Outdated
| <div className={style.menuNavWrapper}> | ||
| <MainNav /> | ||
| </div> | ||
| { noMenu || |
There was a problem hiding this comment.
Rather than running this check inline in the JSP, you could break it out and keep your template code a little cleaner.
const menu = if (hasMenu) <div and mainMenu />
Then in the JSP, something like { menu } instead if the check. Keeping logic out of template code is best for testing when we get that running.
| const { transaction } = this.props | ||
|
|
||
| return ( | ||
| <div> |
There was a problem hiding this comment.
I pointed this out in my big issue the other day, but it's a nice way to break up your concerns if you keep your view JSX in a different component than the connected portion which deals with the redux store. This makes testing easier, separates the implementation from redux in case you want to use the component in a non-connected way, want to remove redux and use a different state management system, or want to keep your testing simple.
This would also allow the component to be written as a stateless functional component which is really lightweight and easy to test.
There was a problem hiding this comment.
Are you saying we should do something like neon-wallet does?
https://github.com/CityOfZion/neon-wallet/blob/dev/app/containers/DisplayWalletKeys/index.js
https://github.com/CityOfZion/neon-wallet/blob/dev/app/containers/DisplayWalletKeys/DisplayWalletKeys.jsx
That separates redux and I'm ok with that. Just haven't planned out how to organize everything quite yet. We could follow their pattern or do something a bit different.
Or you're saying we should completely separate all jsx from even the react state?
There was a problem hiding this comment.
Yeah, the way neon-wallet does it is fine!
There was a problem hiding this comment.
@austinhinderer and I have agreed we'll tackle this as part of refactoring when testing is in place.
| @@ -134,7 +92,7 @@ export default class SendInvoke extends Component { | |||
| } | |||
|
|
|||
| render() { | |||
| const { txid, loading, errorMsg } = this.state | |||
| const { loading, txid, errorMsg } = this.state | |||
There was a problem hiding this comment.
probably just happened with moving code around
|
OK, review comments are handled. I took the Header changes a step further and made it a state-less component. |
Takes care of this one: #14