React router #343

Merged
bones7242 merged 96 commits from react-router into master 2018-02-15 08:02:17 +01:00
11 changed files with 57 additions and 90 deletions
Showing only changes of commit ae5cfa39a1 - Show all commits

View file

@ -11,24 +11,20 @@ class AssetDisplay extends React.Component {
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
error : null,
status: LOCAL_CHECK,
};
this.isLocalFileAvailableOnServer = this.isLocalFileAvailableOnServer.bind(this);
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
this.triggerGetAssetOnServer = this.triggerGetAssetOnServer.bind(this);
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
}
componentDidMount () {
const that = this;
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
this.isLocalFileAvailableOnServer()
.then(isAvailable => {
if (!isAvailable) {
console.log('file is not yet available');
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
that.setState({status: SEARCHING});
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
return that.triggerGetAssetOnServer();
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
this.setState({status: SEARCHING});
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
return this.triggerGetAssetOnServer();
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
}
})
.then(() => {
that.setState({status: AVAILABLE});
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
this.setState({status: AVAILABLE});
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
})
.catch(error => {
that.setState({
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
this.setState({
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
status: UNAVAILABLE,
error : error.message,
});

neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.
neb-b commented 2018-02-05 20:47:37 +01:00 (Migrated from github.com)
Review

Why do you do const that = this?

Why do you do `const that = this`?
neb-b commented 2018-02-05 20:52:52 +01:00 (Migrated from github.com)
Review

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same <AssetDisplay /> it will make these requests again, even if you just made them a second ago

I think this is another piece you can move entirely into redux. Currently if this component is rendered, then a user navigates away and comes back to the same `<AssetDisplay />` it will make these requests again, even if you just made them a second ago
bones7242 commented 2018-02-07 00:13:24 +01:00 (Migrated from github.com)
Review

I had a misunderstanding of how the this context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

I had a misunderstanding of how the `this` context works and when I needed to pass this in to a function manually. I was able to remove it from the app in multiple places where it isn't necessary.

View file

@ -1,5 +1,6 @@
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
import React from 'react';
import PropTypes from 'prop-types';
import { Link } from 'react-router-dom';
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
class AssetInfo extends React.Component {
constructor (props) {
@ -49,8 +50,8 @@ class AssetInfo extends React.Component {
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
<div className="row row--padded row--wide row--no-top">
<div id="show-short-link">
<div className="column column--2 column--med-10">
<a className="link--primary" href={`/${this.props.shortClaimId}/${this.props.name}.${this.props.fileExt}`}><span
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
className="text">Link:</span></a>
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
<Link className="link--primary" to={`/${this.props.shortClaimId}/${this.props.name}.${this.props.fileExt}`}><span
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
className="text">Link:</span></Link>
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
</div>
<div className="column column--8 column--med-10">
<div className="row row--short row--wide">

neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.
neb-b commented 2018-02-05 20:39:44 +01:00 (Migrated from github.com)
Review

This should be a button if it isn't linking anywhere.

This should be a `button` if it isn't linking anywhere.

View file

@ -8,7 +8,6 @@ class Preview extends React.Component {
imgSource : '',
defaultThumbnail: '/assets/img/video_thumb_default.png',
};
this.previewFile = this.previewFile.bind(this);
}
componentDidMount () {
this.previewFile(this.props.file);
@ -22,15 +21,14 @@ class Preview extends React.Component {
}
}
previewFile (file) {
const that = this;
if (file.type !== 'video/mp4') {
const previewReader = new FileReader();
previewReader.readAsDataURL(file);
previewReader.onloadend = function () {
that.setState({imgSource: previewReader.result});
previewReader.onloadend = () => {
this.setState({imgSource: previewReader.result});
};
} else {
that.setState({imgSource: (this.props.thumbnail || this.state.defaultThumbnail)});
this.setState({imgSource: (this.props.thumbnail || this.state.defaultThumbnail)});
}
}
render () {

View file

@ -8,9 +8,6 @@ class ChannelClaimsDisplay extends React.Component {
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.state = {
error: null,
};
this.updateClaimsData = this.updateClaimsData.bind(this);
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.showPreviousResultsPage = this.showPreviousResultsPage.bind(this);
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.showNextResultsPage = this.showNextResultsPage.bind(this);
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
}
componentDidMount () {
const name = this.props.name;
@ -24,18 +21,17 @@ class ChannelClaimsDisplay extends React.Component {
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
}
updateClaimsData (name, longId, page) {
const url = `/api/channel-claims/${name}/${longId}/${page}`;
const that = this;
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
return request(url)
.then(({ success, message, data }) => {
console.log('api/channel-claims response:', data);
if (!success) {
return that.setState({error: message});
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
return this.setState({error: message});
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
}
that.setState({error: null}); // move this error to redux state
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
that.props.onChannelClaimsDataUpdate(data.claims, data.currentPage, data.totalPages, data.totalResults);
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.setState({error: null}); // move this error to redux state
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.props.onChannelClaimsDataUpdate(data.claims, data.currentPage, data.totalPages, data.totalResults);
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
})
.catch((error) => {
that.setState({error: error.message});
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
this.setState({error: error.message});
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
});
}
componentWillUnmount () {

neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)
neb-b commented 2018-02-05 20:26:45 +01:00 (Migrated from github.com)
Review

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch updateClaimsData action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

This might be what you were thinking of doing, but instead of doing the request here, then calling an action to update the data or set an error, just dispatch `updateClaimsData` action which makes the call, then updates the redux state accordingly. I think making an effort to keep all data logic inside of redux files can simplify a lot of components (for the most part)

View file

@ -11,13 +11,8 @@ class ChannelCreateForm extends React.Component {
password: '',
status : null,
};
this.cleanseChannelInput = this.cleanseChannelInput.bind(this);
this.handleChannelInput = this.handleChannelInput.bind(this);
this.handleInput = this.handleInput.bind(this);
this.updateIsChannelAvailable = this.updateIsChannelAvailable.bind(this);
this.checkIsChannelAvailable = this.checkIsChannelAvailable.bind(this);
this.checkIsPasswordProvided = this.checkIsPasswordProvided.bind(this);
this.makePublishChannelRequest = this.makePublishChannelRequest.bind(this);
this.createChannel = this.createChannel.bind(this);
}
cleanseChannelInput (input) {
@ -41,18 +36,17 @@ class ChannelCreateForm extends React.Component {
this.setState({[name]: value});
}
updateIsChannelAvailable (channel) {
const that = this;
const channelWithAtSymbol = `@${channel}`;
request(`/api/channel-is-available/${channelWithAtSymbol}`)
.then(isAvailable => {
if (isAvailable) {
that.setState({'error': null});
this.setState({'error': null});
} else {
that.setState({'error': 'That channel has already been claimed'});
this.setState({'error': 'That channel has already been claimed'});
}
})
.catch((error) => {
that.setState({'error': error.message});
this.setState({'error': error.message});
});
}
checkIsChannelAvailable (channel) {
@ -105,21 +99,20 @@ class ChannelCreateForm extends React.Component {
}
createChannel (event) {
event.preventDefault();
const that = this;
this.checkIsPasswordProvided()
.then(() => {
return that.checkIsChannelAvailable(that.state.channel, that.state.password);
return this.checkIsChannelAvailable(this.state.channel, this.state.password);
})
.then(() => {
that.setState({status: 'We are publishing your new channel. Sit tight...'});
return that.makePublishChannelRequest(that.state.channel, that.state.password);
this.setState({status: 'We are publishing your new channel. Sit tight...'});
return this.makePublishChannelRequest(this.state.channel, this.state.password);
})
.then(result => {
that.setState({status: null});
that.props.onChannelLogin(result.channelName, result.shortChannelId, result.channelClaimId);
this.setState({status: null});
this.props.onChannelLogin(result.channelName, result.shortChannelId, result.channelClaimId);
})
.catch((error) => {
that.setState({'error': error.message, status: null});
this.setState({'error': error.message, status: null});
});
}
render () {

View file

@ -27,22 +27,21 @@ class ChannelLoginForm extends React.Component {
}),
credentials: 'include',
}
const that = this;
request('login', params)
.then(({success, channelName, shortChannelId, channelClaimId, message}) => {
console.log('loginToChannel success:', success);
if (success) {
that.props.onChannelLogin(channelName, shortChannelId, channelClaimId);
this.props.onChannelLogin(channelName, shortChannelId, channelClaimId);
} else {
that.setState({'error': message});
this.setState({'error': message});
};
})
.catch(error => {
console.log('login error', error);
if (error.message) {
that.setState({'error': error.message});
this.setState({'error': error.message});
} else {
that.setState({'error': error});
this.setState({'error': error});
}
});
}

View file

@ -11,9 +11,7 @@ import * as publishStates from 'constants/publish_claim_states';
class PublishForm extends React.Component {
constructor (props) {
super(props);
this.validateChannelSelection = this.validateChannelSelection.bind(this);
this.validatePublishParams = this.validatePublishParams.bind(this);
this.makePublishRequest = this.makePublishRequest.bind(this);
// this.makePublishRequest = this.makePublishRequest.bind(this);
this.publish = this.publish.bind(this);
}
validateChannelSelection () {
@ -53,32 +51,30 @@ class PublishForm extends React.Component {
const uri = '/api/claim-publish';
const xhr = new XMLHttpRequest();
const fd = this.appendDataToFormData(file, metadata);
const that = this;
xhr.upload.addEventListener('loadstart', function () {
that.props.onPublishStatusChange(publishStates.LOAD_START, 'upload started');
xhr.upload.addEventListener('loadstart', () => {
this.props.onPublishStatusChange(publishStates.LOAD_START, 'upload started');
});
xhr.upload.addEventListener('progress', function (e) {
xhr.upload.addEventListener('progress', (e) => {
if (e.lengthComputable) {
const percentage = Math.round((e.loaded * 100) / e.total);
console.log('progress:', percentage);
that.props.onPublishStatusChange(publishStates.LOADING, `${percentage}%`);
this.props.onPublishStatusChange(publishStates.LOADING, `${percentage}%`);
}
}, false);
xhr.upload.addEventListener('load', function () {
xhr.upload.addEventListener('load', () => {
console.log('loaded 100%');
that.props.onPublishStatusChange(publishStates.PUBLISHING, null);
this.props.onPublishStatusChange(publishStates.PUBLISHING, null);
}, false);
xhr.open('POST', uri, true);
xhr.onreadystatechange = function () {
xhr.onreadystatechange = () => {
if (xhr.readyState === 4) {
const response = JSON.parse(xhr.response);
console.log('publish response:', response);
if ((xhr.status === 200) && response.success) {
that.props.onPublishStatusChange(publishStates.SUCCESS, response.data.url);
// redirect to the published asset's show page
that.props.history.push(`/${response.data.claimId}/${response.data.name}`);
this.props.onPublishStatusChange(publishStates.SUCCESS, response.data.url);
this.props.history.push(`/${response.data.claimId}/${response.data.name}`);
} else {
that.props.onPublishStatusChange(publishStates.FAILED, response.message);
this.props.onPublishStatusChange(publishStates.FAILED, response.message);
}
}
};
@ -106,7 +102,6 @@ class PublishForm extends React.Component {
fd.append('file', file);
for (var key in metadata) {
if (metadata.hasOwnProperty(key)) {
console.log('adding form data', key, metadata[key]);
fd.append(key, metadata[key]);
}
}
@ -115,21 +110,20 @@ class PublishForm extends React.Component {
publish () {
console.log('publishing file');
// publish the asset
const that = this;
this.validateChannelSelection()
.then(() => {
return that.validatePublishParams();
return this.validatePublishParams();
})
neb-b commented 2018-02-05 20:15:43 +01:00 (Migrated from github.com)
Review

Not really a comment on this PR but it might be worth looking into fetch instead of dealing with xhr

Not really a comment on this PR but it might be worth looking into `fetch` instead of dealing with `xhr`
bones7242 commented 2018-02-09 20:21:03 +01:00 (Migrated from github.com)
Review

Cool. Yeah, I replaced most xhr requests with fetch in the app. I kept this one for the publish request because I wanted to set event listeners on the request and wasn't sure if I could do that with fetch. I'll look into that.

Cool. Yeah, I replaced most xhr requests with fetch in the app. I kept this one for the publish request because I wanted to set event listeners on the request and wasn't sure if I could do that with fetch. I'll look into that.
.then(() => {
const metadata = that.createMetadata();
const metadata = this.createMetadata();
// publish the claim
return that.makePublishRequest(that.props.file, metadata);
return this.makePublishRequest(this.props.file, metadata);
})
.then(() => {
that.props.onPublishStatusChange('publish request made');
this.props.onPublishStatusChange('publish request made');
})
.catch((error) => {
that.props.onPublishSubmitError(error.message);
this.props.onPublishSubmitError(error.message);
});
}
render () {

View file

@ -9,8 +9,6 @@ class PublishThumbnailInput extends React.Component {
thumbnailInput : '',
}
this.handleInput = this.handleInput.bind(this);
this.urlIsAnImage = this.urlIsAnImage.bind(this);
this.testImage = this.testImage.bind(this);
this.updateVideoThumb = this.updateVideoThumb.bind(this);
}
handleInput (event) {
@ -38,22 +36,21 @@ class PublishThumbnailInput extends React.Component {
}
updateVideoThumb (event) {
const imageUrl = event.target.value;
const that = this;
if (this.urlIsAnImage(imageUrl)) {
this.testImage(imageUrl, 3000)
.then(() => {
console.log('thumbnail is a valid image');
that.props.onThumbnailChange('thumbnail', imageUrl);
that.setState({thumbnailError: null});
this.props.onThumbnailChange('thumbnail', imageUrl);
this.setState({thumbnailError: null});
})
.catch(error => {
console.log('encountered an error loading thumbnail image url:', error);
that.props.onThumbnailChange('thumbnail', null);
that.setState({thumbnailError: 'That is an invalid image url'});
this.props.onThumbnailChange('thumbnail', null);
this.setState({thumbnailError: 'That is an invalid image url'});
});
} else {
that.props.onThumbnailChange('thumbnail', null);
that.setState({thumbnailError: null});
this.props.onThumbnailChange('thumbnail', null);
this.setState({thumbnailError: null});
}
}
render () {

View file

@ -6,9 +6,6 @@ class PublishUrlInput extends React.Component {
constructor (props) {
super(props);
this.handleInput = this.handleInput.bind(this);
this.cleanseInput = this.cleanseInput.bind(this);
this.setClaimNameFromFileName = this.setClaimNameFromFileName.bind(this);
this.checkClaimIsAvailable = this.checkClaimIsAvailable.bind(this);
}
componentDidMount () {
if (!this.props.claim || this.props.claim === '') {
@ -40,18 +37,17 @@ class PublishUrlInput extends React.Component {
this.props.onClaimChange(cleanClaimName);
}
checkClaimIsAvailable (claim) {
const that = this;
request(`/api/claim-is-available/${claim}`)
.then(isAvailable => {
// console.log('checkClaimIsAvailable request response:', isAvailable);
if (isAvailable) {
that.props.onUrlError(null);
this.props.onUrlError(null);
} else {
that.props.onUrlError('That url has already been claimed');
this.props.onUrlError('That url has already been claimed');
}
})
.catch((error) => {
that.props.onUrlError(error.message);
this.props.onUrlError(error.message);
});
}
render () {

View file

@ -36,10 +36,9 @@ class ShowAsset extends React.Component {
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
body: JSON.stringify(body),
}
// make request
const that = this;
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
this.getLongClaimId(params)
.then(claimLongId => {
return Promise.all([that.getShortClaimId(claimLongId, name), that.getClaimData(claimLongId, name)]);
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
return Promise.all([this.getShortClaimId(claimLongId, name), this.getClaimData(claimLongId, name)]);
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
})
.then(([shortId, claimData]) => {
this.setState({error: null}); // note: move this to redux level

neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.
neb-b commented 2018-02-13 06:13:36 +01:00 (Migrated from github.com)
Review

I think you are still creating more work than necessary with this. In my opinion previousRequest shouldn't even exist. In the mapStateToProps you should be able to map the asset from your state into the component. If !asset then make the request.

I also think onShowNewAsset and onNewRequest can be combined. More specifically I don't think onShowNewAsset is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".

I think you are still creating more work than necessary with this. In my opinion `previousRequest` shouldn't even exist. In the `mapStateToProps` you should be able to map the `asset` from your state into the component. If `!asset` then make the request. I also think `onShowNewAsset` and `onNewRequest` can be combined. More specifically I don't think `onShowNewAsset` is needed. It might just be my lack of understanding with the current data flow, but you shouldn't need to manually say "show this asset". A better approach would be "select the asset with this id".
bones7242 commented 2018-02-14 02:17:20 +01:00 (Migrated from github.com)
Review

Ok, I think I'm getting closer. I was able to do away with onShowNewAsset and combine the needed logic from its action (retrieving the asset's claim data) into onNewRequest. That allowed me to remove previousRequest from the props I am passing to the <ShowAsset /> component. However, I am still checking for a previousRequest in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full claimId from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

Ok, I think I'm getting closer. I was able to do away with `onShowNewAsset` and combine the needed logic from its action (retrieving the asset's claim data) into `onNewRequest`. That allowed me to remove `previousRequest` from the props I am passing to the `<ShowAsset />` component. However, I am still checking for a `previousRequest` in the mapStateToProps function. Do you see a way to avoid that step altogether? The reason for storing and checking the previous requests is to avoid having to retrieve new information for a request that was already made (i.e. to avoid having to request the full `claimId` from the server). I'm trying to figure out if that can be skipped or consolidated, but I am not sure how.

View file

@ -9,7 +9,6 @@ class ShowChannel extends React.Component {
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
this.state = {
error: null,
};
this.getAndStoreChannelData = this.getAndStoreChannelData.bind(this);
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
}
componentDidMount () {
this.getAndStoreChannelData(this.props.requestName, this.props.requestId);
@ -22,18 +21,17 @@ class ShowChannel extends React.Component {
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
getAndStoreChannelData (name, id) {
if (!id) id = 'none';
const url = `/api/channel-data/${name}/${id}`;
const that = this;
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
return request(url)
.then(({ success, message, data }) => {
console.log('api/channel-data response:', data);
if (!success) {
return that.setState({error: message});
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
return this.setState({error: message});
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
}
that.setState({error: null}); // note: store this error at app level also
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
that.props.onChannelDataUpdate(data.channelName, data.longChannelClaimId, data.shortChannelClaimId);
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
this.setState({error: null}); // note: store this error at app level also
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
this.props.onChannelDataUpdate(data.channelName, data.longChannelClaimId, data.shortChannelClaimId);
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
})
.catch((error) => {
that.setState({error: error.message});
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
this.setState({error: error.message});
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
});
}
componentWillUnmount () {

neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.
neb-b commented 2018-02-13 06:15:15 +01:00 (Migrated from github.com)
Review

Same comments above about previousRequest. I think a more understandable approach would just be:

if (!channel) this.props.onNewChannelRequest(...)
Same comments above about `previousRequest`. I think a more understandable approach would just be: ``` if (!channel) this.props.onNewChannelRequest(...) ```
bones7242 commented 2018-02-14 02:18:19 +01:00 (Migrated from github.com)
Review

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.

See above re: previous request. I mostly fixed this, but not sure if more consolidation can be done.