let users open urls from dapps view #4042
Conversation
| import styles from './urlButton.css'; | ||
|
|
||
| class UrlButton extends Component { | ||
| static propTypes = { |
There was a problem hiding this comment.
router is actually also available on the context, so we don't need the withRouter wrapper. Could just do static contextTypes = { router: PropTypes.object.isRequired }, e.g. Same as what we do in views/Settings/settings.js
There was a problem hiding this comment.
Followed the a react-router guide, which says this:
Prior to 2.4.0, you could access the
routerobject viathis.context. This is still true, butcontextis often times a difficult and error-prone API to work with.
In order to more easily access the
routerobject, awithRouterhigher-order component has been added as the new primary means of access.
It's important to note this is not a deprecation of the
contextAPI. As long as React supportsthis.contextin its current form, any code written for that API will continue to work. We will continue to use it internally and you can continue to write in that format, if you want. We think this new HoC is nicer and easier, and will be using it in documentation and examples, but it is not a hard requirement to switch.
| onBlur={ this.hideInput } | ||
| onFocus={ this.showInput } | ||
| onSubmit={ this.inputOnSubmit } | ||
| style={ { display: 'inline-block', width: '20em' } } |
There was a problem hiding this comment.
Small picky thing, but wouldn't it make more sense to define the style as a constant and then just reference it here?
| @@ -88,6 +89,9 @@ class Dapps extends Component { | |||
| defaultMessage='Decentralized Applications' /> | |||
| } | |||
| buttons={ [ | |||
| <UrlButton | |||
There was a problem hiding this comment.
Think this could just be a single line since there is only one attribute
| }); | ||
| }; | ||
|
|
||
| handleIframeLoad = (ev) => { | ||
| iframeOnLoad = () => { |
There was a problem hiding this comment.
Possibly onIframeLoad since it is an event?
There was a problem hiding this comment.
But it is a load even on the iframe, not a iframeLoad event. I think the current form is more expressive. Still, having all even handlers begin with on is a fair point.
|
Some small comments, but looking quite good. (Just want to play a bit to see if anything pops out) |
|
One remark though: the protocol is not inferred, hence typing |
ngotchac
added
A5-grumble 🔥
derhuerst
added
A0-pleasereview 🤓
| let host = parsed.host; | ||
| let path = parsed.path; | ||
| if (!host) { | ||
| host = parsed.path.split(pathSep).slice(0, 1); |
There was a problem hiding this comment.
Isn't this returning an array ? It's fine with the cast to String though, but still...
| let path = parsed.path; | ||
| if (!host) { | ||
| host = parsed.path.split(pathSep).slice(0, 1); | ||
| path = parsed.path.split(pathSep).slice(1).join(pathSep); |
There was a problem hiding this comment.
Isn't there missing the first / then ? Try reddit.com/r/ethereum
Also, what's the point of using pathSep ? Should always be / with URLs, no ?
There was a problem hiding this comment.
Also, what's the point of using pathSep ? Should always be / with URLs, no ?
You're right. I am so much into "always use path.sep to make modules cross-platform" that i didn't think about it.
ngotchac
added
A5-grumble 🔥
|
Changes Unknown when pulling f402ca7 on jr-url-input into ** on master**. |
derhuerst
added
A0-pleasereview 🤓
ngotchac
added
A8-looksgood 🦄
Fixes #3957.