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 42 additions and 92 deletions
Showing only changes of commit 51c50d7f5d - Show all commits

View file

@ -1,14 +1,14 @@
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
import * as actions from 'constants/show_action_types';
// basic request parsing
export function updateRequestError (error) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onRequestError (error) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
return {
type: actions.REQUEST_UPDATE_ERROR,
data: error,
};
}
export function updateRequestWithChannelRequest (name, id) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onParsedChannelRequest (name, id) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
const requestId = `cr#${name}#${id}`;
return {
type: actions.REQUEST_UPDATE_CHANNEL,
@ -16,7 +16,7 @@ export function updateRequestWithChannelRequest (name, id) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
};
};
export function updateRequestWithAssetRequest (name, id, channelName, channelId, extension) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onParsedAssetRequest (name, id, channelName, channelId, extension) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
const requestId = `ar#${name}#${id}#${channelName}#${channelId}`;
return {
type: actions.REQUEST_UPDATE_ASSET,
@ -37,7 +37,7 @@ export function updateRequestWithAssetRequest (name, id, channelName, channelId,
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
// asset actions
export function newAssetRequest (id, name, modifier) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onNewAssetRequest (id, name, modifier) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
return {
type: actions.ASSET_REQUEST_NEW,
data: { id, name, modifier },
@ -60,7 +60,7 @@ export function addAssetToAssetList (id, error, name, claimId, shortId, claimDat
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
// channel actions
export function newChannelRequest (id, name, channelId) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onNewChannelRequest (id, name, channelId) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
return {
type: actions.CHANNEL_REQUEST_NEW,
data: {id, name, channelId},
@ -83,7 +83,7 @@ export function addNewChannelToChannelList (id, name, shortId, longId, claimsDat
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
// update channel data
export function updateChannelClaimsAsync (channelKey, name, longId, page) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
export function onUpdateChannelClaims (channelKey, name, longId, page) {
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
return {
type: actions.CHANNEL_CLAIMS_UPDATE_ASYNC,
data: {channelKey, name, longId, page},

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

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async
neb-b commented 2018-02-05 20:57:15 +01:00 (Migrated from github.com)
Review

I think generally the pattern is that an action is { type: "some string", data: { name, id... } } just to keep things consistent. data can be an object or a string, but I think it's helpful to put everything inside of that

I think generally the pattern is that an action is `{ type: "some string", data: { name, id... } }` just to keep things consistent. `data` can be an object or a string, but I think it's helpful to put everything inside of that
neb-b commented 2018-02-13 06:05:56 +01:00 (Migrated from github.com)
Review

This probably shouldn't be called XXX_ASYNC since it isn't async

This probably shouldn't be called `XXX_ASYNC` since it isn't async

View file

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

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
import { connect } from 'react-redux';
import { updateChannelClaimsAsync } from 'actions/show';
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
import { onUpdateChannelClaims } from 'actions/show';
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
import View from './view';
const mapStateToProps = ({ show }) => {
@ -15,11 +15,9 @@ const mapStateToProps = ({ show }) => {
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
};
};
const mapDispatchToProps = dispatch => {
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
const mapDispatchToProps = () => {
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
return {
onChannelPageUpdate: (channelKey, name, longId, page) => {
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
dispatch(updateChannelClaimsAsync(channelKey, name, longId, page));
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
},
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
onUpdateChannelClaims,
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
};
};

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

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error
neb-b commented 2018-02-05 20:27:54 +01:00 (Migrated from github.com)
Review

Will these nested values always exist?

Will these nested values always exist?
bones7242 commented 2018-02-07 07:58:43 +01:00 (Migrated from github.com)
Review

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?

They will always exist when ChannelClaimsDisplay is rendered, unless that should change... I am updated it to be destructured, is that what you were thinking?
neb-b commented 2018-02-07 08:10:06 +01:00 (Migrated from github.com)
Review

I was just wondering if there would ever be a case when show is undefined. Which would cause an errror. cannot read property 'showChannel of undefined`.

Or if any of those children would be undefined which would throw an error

I was just wondering if there would ever be a case when `show` is undefined. Which would cause an errror. `cannot read property 'showChannel` of undefined`. Or if any of those children would be undefined which would throw an error

View file

@ -19,7 +19,7 @@ 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)
}
showNewPage (page) {
const { channelKey, channel: { name, longId } } = this.props;
this.props.onChannelPageUpdate(channelKey, name, longId, page);
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.onUpdateChannelClaims(channelKey, name, longId, page);
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)
}
render () {
const { channel: { claimsData: { claims, currentPage, totalPages } } } = this.props;

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

@ -1,10 +1,9 @@
import { connect } from 'react-redux';
import View from './view';
import { newAssetRequest, showNewAsset } from 'actions/show';
import { onNewAssetRequest } from 'actions/show';
const mapStateToProps = ({ show }) => {
// select request info
const requestType = show.request.type;
const requestId = show.request.id;
const requestName = show.request.data.name;
const requestModifier = show.request.data.modifier;
@ -17,10 +16,8 @@ const mapStateToProps = ({ show }) => {
const assetKey = `a#${previousRequest.name}#${previousRequest.claimId}`; // note: just store this in the request
asset = assetList[assetKey] || null;
};
// console.log('previousRequest:', previousRequest, 'asset:', asset, 'asset list', assetList);
// return props
return {
requestType,
requestId,
requestName,
requestModifier,
@ -29,12 +26,9 @@ const mapStateToProps = ({ show }) => {
};
};
const mapDispatchToProps = dispatch => {
const mapDispatchToProps = () => {
return {
// request
onNewRequest: (id, name, modifier) => {
dispatch(newAssetRequest(id, name, modifier));
},
onNewAssetRequest,
};
};

View file

@ -3,36 +3,18 @@ import ErrorPage from 'components/ErrorPage';
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.
import ShowAssetLite from 'components/ShowAssetLite';
import ShowAssetDetails from 'components/ShowAssetDetails';
import { ASSET } from 'constants/show_request_types';
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.
function requestIsAnAssetRequest ({ requestType }) {
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 requestType === ASSET;
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.
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.
class ShowAsset extends React.Component {
componentDidMount () {
const { asset, requestId, requestName, requestModifier } = this.props;
if (!asset) { // case: the asset info does not exist
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 this.props.onNewRequest(requestId, requestName, requestModifier);
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.
if (!asset) {
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 this.props.onNewAssetRequest(requestId, requestName, requestModifier);
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.
};
}
componentWillReceiveProps (nextProps) {
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.
if (requestIsAnAssetRequest(nextProps)) {
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.
const { asset, requestId, requestName, requestModifier } = nextProps;
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.
if (!asset) { // case: the asset info does not exist
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 this.props.onNewRequest(requestId, requestName, requestModifier);
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.
}
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.
render () {
const {asset, requestExtension} = this.props;
if (asset) {
if (requestExtension) {
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 <ShowAssetLite/>;
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.
return <ShowAssetDetails/>;
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.
;
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 requestExtension ? <ShowAssetLite/> : <ShowAssetDetails/>;
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.
return (
<ErrorPage error={'loading asset data...'}/>
);

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

@ -1,11 +1,10 @@
import { connect } from 'react-redux';
import { newChannelRequest } from 'actions/show';
import { onNewChannelRequest } from 'actions/show';
import View from './view';
const mapStateToProps = ({ show }) => {
// select request info
const requestId = show.request.id;
const requestType = show.request.type;
const requestChannelName = show.request.data.name;
const requestChannelId = show.request.data.id;
// select request
@ -18,18 +17,15 @@ const mapStateToProps = ({ show }) => {
}
return {
requestId,
requestType,
requestChannelName,
requestChannelId,
channel,
};
};
const mapDispatchToProps = dispatch => {
const mapDispatchToProps = () => {
return {
onNewChannelRequest (requestId, requestChannelName, requestChannelId) {
dispatch(newChannelRequest(requestId, requestChannelName, requestChannelId));
},
onNewChannelRequest,
};
};

View file

@ -3,12 +3,6 @@ import ErrorPage from 'components/ErrorPage';
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.
import NavBar from 'containers/NavBar';
import ChannelClaimsDisplay from 'containers/ChannelClaimsDisplay';
import { CHANNEL } from 'constants/show_request_types';
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.
function requestIsAChannelRequest ({ requestType }) {
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 requestType === CHANNEL;
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.
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.
class ShowChannel extends React.Component {
componentDidMount () {
const { channel, requestId, requestChannelName, requestChannelId } = this.props;
@ -16,14 +10,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.
return this.props.onNewChannelRequest(requestId, requestChannelName, requestChannelId);
}
}
componentWillReceiveProps (nextProps) {
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.
if (requestIsAChannelRequest(nextProps)) {
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.
const { channel, requestId, requestChannelName, requestChannelId } = nextProps;
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.
if (!channel) {
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.props.onNewChannelRequest(requestId, requestChannelName, requestChannelId);
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.
}
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.
render () {
const { channel } = this.props;
if (channel) {
@ -33,9 +19,9 @@ 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.
<NavBar/>
<div className="row row--tall row--padded">
<div className="column column--10">
<h2>channel name: {name ? name : 'loading...'}</h2>
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.
<p className={'fine-print'}>full channel id: {longId ? longId : 'loading...'}</p>
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.
<p className={'fine-print'}>short channel id: {shortId ? shortId : 'loading...'}</p>
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.
<h2>channel name: {name || 'loading...'}</h2>
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.
<p className={'fine-print'}>full channel id: {longId || 'loading...'}</p>
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.
<p className={'fine-print'}>short channel id: {shortId || 'loading...'}</p>
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.
</div>
<div className="column column--10">
<ChannelClaimsDisplay />

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.

View file

@ -1,5 +1,5 @@
import { connect } from 'react-redux';
import { updateRequestError, updateRequestWithChannelRequest, updateRequestWithAssetRequest } from 'actions/show';
import { onRequestError, onParsedChannelRequest, onParsedAssetRequest } from 'actions/show';
import View from './view';
const mapStateToProps = ({ show }) => {
@ -9,17 +9,11 @@ const mapStateToProps = ({ show }) => {
};
};
const mapDispatchToProps = dispatch => {
const mapDispatchToProps = () => {
return {
onRequestError: (error) => {
dispatch(updateRequestError(error));
},
onChannelRequest: (name, id) => {
dispatch(updateRequestWithChannelRequest(name, id));
},
onAssetRequest: (name, id, channelName, channelId, extension) => {
dispatch(updateRequestWithAssetRequest(name, id, channelName, channelId, extension));
},
onRequestError,
onParsedChannelRequest,
onParsedAssetRequest,
};
};

View file

@ -42,9 +42,9 @@ class ShowPage extends React.Component {
}
// update the store
if (isChannel) {
return this.props.onAssetRequest(claimName, null, channelName, channelClaimId, extension);
return this.props.onParsedAssetRequest(claimName, null, channelName, channelClaimId, extension);
} else {
return this.props.onAssetRequest(claimName, claimId, null, null, extension);
return this.props.onParsedAssetRequest(claimName, claimId, null, null, extension);
}
}
parseAndUpdateClaimOnly (claim) {
@ -58,7 +58,7 @@ class ShowPage extends React.Component {
}
// return early if this request is for a channel
if (isChannel) {
return this.props.onChannelRequest(channelName, channelClaimId);
return this.props.onParsedChannelRequest(channelName, channelClaimId);
}
// if not for a channel, parse the claim request
let claimName, extension; // if I am destructuring below, do I still need to declare these here?
@ -67,7 +67,7 @@ class ShowPage extends React.Component {
} catch (error) {
return this.props.onRequestError(error.message);
}
this.props.onAssetRequest(claimName, null, null, null, extension);
this.props.onParsedAssetRequest(claimName, null, null, null, extension);
}
render () {
const { error, requestType } = this.props;

View file

@ -1,6 +1,6 @@
import { call, put, takeLatest } from 'redux-saga/effects';
import * as actions from 'constants/show_action_types';
import { addRequestToAssetRequests, updateRequestError, addAssetToAssetList } from 'actions/show';
import { addRequestToAssetRequests, onRequestError, addAssetToAssetList } from 'actions/show';
import { getLongClaimId, getShortId, getClaimData } from 'api/assetApi';
function* newAssetRequest (action) {
@ -12,7 +12,7 @@ function* newAssetRequest (action) {
({data: longId} = yield call(getLongClaimId, name, modifier));
} catch (error) {
console.log('error:', error);
return yield put(updateRequestError(error.message));
return yield put(onRequestError(error.message));
}
// put action to add request to asset request list
yield put(addRequestToAssetRequests(id, null, name, longId));
@ -22,7 +22,7 @@ function* newAssetRequest (action) {
try {
({data: shortId} = yield call(getShortId, name, longId));
} catch (error) {
return yield put(updateRequestError(error.message));
return yield put(onRequestError(error.message));
}
// get asset claim data
console.log(`getting asset claim data ${name} ${longId}`);
@ -30,13 +30,13 @@ function* newAssetRequest (action) {
try {
({data: claimData} = yield call(getClaimData, name, longId));
} catch (error) {
return yield put(updateRequestError(error.message));
return yield put(onRequestError(error.message));
}
// put action to add asset to asset list
const assetKey = `a#${name}#${longId}`;
yield put(addAssetToAssetList(assetKey, null, name, longId, shortId, claimData));
// clear any errors in request error
yield put(updateRequestError(null));
yield put(onRequestError(null));
};
export function* watchNewAssetRequest () {

View file

@ -1,6 +1,6 @@
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
import { call, put, takeLatest } from 'redux-saga/effects';
import * as actions from 'constants/show_action_types';
import { addNewChannelToChannelList, addRequestToChannelRequests, updateRequestError, updateChannelClaims } from 'actions/show';
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
import { addNewChannelToChannelList, addRequestToChannelRequests, onRequestError, updateChannelClaims } from 'actions/show';
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
import { getChannelClaims, getChannelData } from 'api/channelApi';
function* getNewChannelAndUpdateChannelList (action) {
@ -11,7 +11,7 @@ function* getNewChannelAndUpdateChannelList (action) {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
try {
({ data: {longChannelClaimId: longId, shortChannelClaimId: shortId} } = yield call(getChannelData, name, channelId));
} catch (error) {
return yield put(updateRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
return yield put(onRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
// store the request in the channel requests list
yield put(addRequestToChannelRequests(id, null, name, longId, shortId));
@ -21,13 +21,13 @@ function* getNewChannelAndUpdateChannelList (action) {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
try {
({ data: claimsData } = yield call(getChannelClaims, name, longId, 1));
} catch (error) {
return yield put(updateRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
return yield put(onRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
// store the channel data in the channel list
const channelKey = `c#${name}#${longId}`;
yield put(addNewChannelToChannelList(channelKey, name, shortId, longId, claimsData));
// clear any request errors
yield put(updateRequestError(null));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
yield put(onRequestError(null));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
export function* watchNewChannelRequest () {
@ -40,7 +40,7 @@ function* getNewClaimsAndUpdateChannel (action) {
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
try {
({ data: claimsData } = yield call(getChannelClaims, name, longId, page));
} catch (error) {
return yield put(updateRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
return yield put(onRequestError(error.message));
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
}
yield put(updateChannelClaims(channelKey, claimsData));
}

neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue
neb-b commented 2018-02-09 22:53:34 +01:00 (Migrated from github.com)
Review

I don't have any experience using redux-saga so I might just not understand what is happening.

Generally you want XX_SUCCESS and XX_FAIL actions separated. That makes it a lot easier handling the data on the reducer.

I don't have any experience using `redux-saga` so I might just not understand what is happening. Generally you want `XX_SUCCESS` and `XX_FAIL` actions separated. That makes it a lot easier handling the data on the reducer.
neb-b commented 2018-02-09 22:55:57 +01:00 (Migrated from github.com)
Review

If these aren't being used anywhere else, I don't think they need to be separated.

IMO it would make these saga files easier to understand/follow, but not a big issue

If these aren't being used anywhere else, I don't think they need to be separated. IMO it would make these saga files easier to understand/follow, but not a big issue