Discovery #2

2019-07-15T04:49:52+02:00

akinwale commented

2019-07-15 04:49:52 +02:00

(Migrated from github.com)

No description provided.

neb-b (Migrated from github.com) reviewed 2019-07-15 04:49:52 +02:00

kauffj (Migrated from github.com) requested changes 2019-07-24 20:06:23 +02:00

src/component/AppNavigator.js

kauffj (Migrated from github.com) commented

2019-07-24 19:11:14 +02:00

It doesn't look like this or the line below introduced these magic values so it's not mandatory to fix, but ideally they would not be used here.

src/component/channelIconItem/view.js

kauffj (Migrated from github.com) commented

2019-07-24 19:13:25 +02:00

I assume the above line means that some claims have a thumbnail property of ' ' or similar.

Would it make sense to trim this value somewhere in lbry-redux or elsewhere in the data model instead? Otherwise, this line would theoretically have to be repeated whenever working with this property, which is not intuitive/expected.

I assume the above line means that some claims have a thumbnail property of `' '` or similar. Would it make sense to `trim` this value somewhere in `lbry-redux` or elsewhere in the data model instead? Otherwise, this line would theoretically have to be repeated whenever working with this property, which is not intuitive/expected.

kauffj (Migrated from github.com) commented

2019-07-24 19:51:55 +02:00

yep, just saw this here too https://github.com/lbryio/lbry-react-native/pull/2/files#diff-5cf471858420cda219b3d4720a301029R41

please move to redux

yep, just saw this here too https://github.com/lbryio/lbry-react-native/pull/2/files#diff-5cf471858420cda219b3d4720a301029R41 please move to redux

src/component/claimList/index.js

kauffj (Migrated from github.com) commented

2019-07-24 19:30:47 +02:00

https://lbryians.slack.com/archives/C81FGKR51/p1563989434031800

kauffj (Migrated from github.com) commented

2019-07-24 19:31:24 +02:00

If "10" is a default page size in multiple places, please abstract it as a shared constant

src/component/claimList/view.js

					
				@ -0,0 +62,4 @@

				    } = this.props;

				    const { orderBy, tags, channelIds, trendingForAll } = nextProps;

				    if (

				      !_.isEqual(orderBy, prevOrderBy) ||

kauffj (Migrated from github.com) commented

2019-07-24 19:38:35 +02:00

I dislike bringing in a dependency just to compare objects :/

If we're not making heavy usage of lodash, I'd propose just bringing in this function (that is, just copy/paste it - yes, this is sometimes better than a dependency).

If this is only being used to determine if non-nested objects contain identical values and keys, I suspect it can be done in just a few lines ourselves

I dislike bringing in a dependency just to compare objects :/ If we're not making heavy usage of lodash, I'd propose just bringing in this function (that is, just copy/paste it - yes, this is sometimes better than a dependency). If this is only being used to determine if non-nested objects contain identical values and keys, I suspect it can be done in just a few lines ourselves

src/component/claimList/view.js

					
				@ -0,0 +84,4 @@

				            options.channel_ids = channelIds;

				          }

				          if (trendingForAll) {

				            this.setState({ trendingForAllView: true });

kauffj (Migrated from github.com) commented

2019-07-24 19:46:49 +02:00

if both tendingForAll and channelIds can be true it results in two calls to setState (and thus it's better to build an object)

if it shouldn't be possible for both to be true, I'd make it an else just to be clearer that they are exclusive conditions

it could also make sense to write a "stupid" (simple) claimList component that only receives claims, and then create a ChannelClaimList and StreamClaimList component that performs all component logic, then passes the resulting uris/claims to the stupid version

if both `tendingForAll` and `channelIds` can be true it results in two calls to `setState` (and thus it's better to build an object) if it shouldn't be possible for both to be true, I'd make it an else just to be clearer that they are exclusive conditions it could also make sense to write a "stupid" (simple) claimList component that only receives claims, and then create a ChannelClaimList and StreamClaimList component that performs all component logic, then passes the resulting uris/claims to the stupid version

src/component/modalTagSelector/view.js

kauffj (Migrated from github.com) commented

2019-07-24 19:49:40 +02:00

missing __

missing `__`

kauffj (Migrated from github.com) commented

2019-07-24 19:50:37 +02:00

why are we calling toLowerCase only here? tag display should be consistent

IMO either lowercase always (and thus move to redux) or display as they are with mixed capitalization even in the toast

why are we calling `toLowerCase` only here? tag display should be consistent IMO either lowercase always (and thus move to redux) or display as they are with mixed capitalization even in the toast

src/component/suggestedSubscriptions/index.js

kauffj (Migrated from github.com) commented

2019-07-24 19:54:03 +02:00

reading logic below, I think you can do:

loading: selectIsFetchingSuggested(state) || selectFetchingClaimSearch(state)

reading logic below, I think you can do: ``` loading: selectIsFetchingSuggested(state) || selectFetchingClaimSearch(state) ```

src/component/suggestedSubscriptions/view.js

					
				@ -5,3 +4,4 @@

				import __, { navigateToUri } from 'utils/helper';

				import SubscribeButton from 'component/subscribeButton';

				import SuggestedSubscriptionItem from 'component/suggestedSubscriptionItem';

				import Colors from 'styles/colors';

kauffj (Migrated from github.com) commented

2019-07-24 19:52:45 +02:00

i18n

src/component/suggestedSubscriptions/view.js

					
				@ -13,0 +23,4 @@

				  }

				  buildSections = () => {

				    const { suggested, claimSearchUris } = this.props;

kauffj (Migrated from github.com) commented

2019-07-24 19:52:36 +02:00

why shuffle?

src/constants.js

kauffj (Migrated from github.com) commented

2019-07-24 19:55:52 +02:00

since these are global constants, this should probably be CLAIM_SEARCH_SORT_BY_ITEMS or similar

since these are global constants, this should probably be `CLAIM_SEARCH_SORT_BY_ITEMS` or similar

kauffj (Migrated from github.com) commented

2019-07-24 19:56:13 +02:00

also, i18n if it's possible to do it here

kauffj (Migrated from github.com) commented

2019-07-24 20:01:49 +02:00

after looking lower, I think you want something like this

SORT_BY_NEW: 'new',
SORT_BY_HOT: 'hot'
SORT_BY_TOP: 'top'

CLAIM_SEARCH_SORT_BY: {
  SORT_BY_HOT: { icon: 'fire-alt', name: SORT_BY_HOT, label: __('Hot Content') }
  ...
}

after looking lower, I think you want something like this ``` SORT_BY_NEW: 'new', SORT_BY_HOT: 'hot' SORT_BY_TOP: 'top' CLAIM_SEARCH_SORT_BY: { SORT_BY_HOT: { icon: 'fire-alt', name: SORT_BY_HOT, label: __('Hot Content') } ... } ```

src/page/discover/view.js

kauffj (Migrated from github.com) commented

2019-07-24 19:57:14 +02:00

we have a find and replace spill in aisle 5

src/page/discover/view.js

kauffj (Migrated from github.com) commented

2019-07-24 19:58:07 +02:00

we lose the value of constant usage if we're just repeating the strings anyway

src/page/discover/view.js

kauffj (Migrated from github.com) commented

2019-07-24 19:58:52 +02:00

i18n

kauffj (Migrated from github.com) commented

2019-07-24 20:06:12 +02:00

appears to match https://github.com/lbryio/lbry-react-native/pull/2/files#diff-b0a4f62a7317c2f3fb4521b1ce85991cR186

src/page/subscriptions/view.js

kauffj (Migrated from github.com) commented

2019-07-24 20:00:19 +02:00

🤢

:nauseated_face:

kauffj (Migrated from github.com) commented

2019-07-24 20:02:33 +02:00

https://github.com/lbryio/lbry-react-native/pull/2/files#diff-a7748843fbda5167d065231e3a2e7a1fR85 is more constructive than the above emoji

src/page/subscriptions/view.js

kauffj (Migrated from github.com) commented

2019-07-24 20:02:44 +02:00

clean up

kauffj (Migrated from github.com) commented

2019-07-24 20:03:34 +02:00

this looks like bad usage of a magic value

can this be tracked as independent boolean?

this looks like bad usage of a magic value can this be tracked as independent boolean?

src/page/tag/view.js

kauffj (Migrated from github.com) commented

2019-07-24 20:03:57 +02:00

.

src/page/trending/view.js

					
				@ -15,3 +29,4 @@

				  didFocusListener;

				  componentWillMount() {

kauffj (Migrated from github.com) commented

2019-07-24 20:05:11 +02:00

use same pattern as above

also if all usage of values is confined to a single file I don't feel nearly as strongly about using constant patterns

the insistence on constant patterns is significantly driven by making inter-dependencies clear and explicit

use same pattern as above also if all usage of values is confined to a single file I don't feel nearly as strongly about using constant patterns the insistence on constant patterns is significantly driven by making inter-dependencies clear and explicit

akinwale (Migrated from github.com) reviewed 2019-07-26 08:39:18 +02:00

src/component/claimList/view.js

					
				@ -0,0 +62,4 @@

				    } = this.props;

				    const { orderBy, tags, channelIds, trendingForAll } = nextProps;

				    if (

				      !_.isEqual(orderBy, prevOrderBy) ||

akinwale (Migrated from github.com) commented

2019-07-26 08:39:18 +02:00

I only decided to make use of the dependency since it was already being imported by a different module.

akinwale (Migrated from github.com) reviewed 2019-07-26 08:41:33 +02:00

src/component/claimList/view.js

					
				@ -0,0 +84,4 @@

				            options.channel_ids = channelIds;

				          }

				          if (trendingForAll) {

				            this.setState({ trendingForAllView: true });

akinwale (Migrated from github.com) commented

2019-07-26 08:41:33 +02:00

It's not possible for both to be true. I initially created the ClaimList component just for displaying content for a tag or set of tags, but then I realised I needed to have the subscriptions (using channelIds) and then eventually trending for all.

akinwale (Migrated from github.com) reviewed 2019-07-26 08:43:53 +02:00

src/component/modalTagSelector/view.js

akinwale (Migrated from github.com) commented

2019-07-26 08:43:52 +02:00

This is a habit I picked up for whenever I want to compare strings making sure they're case insensitive first. I believe the names in followedTags are always lowercase.

akinwale (Migrated from github.com) reviewed 2019-07-26 08:50:21 +02:00

src/component/suggestedSubscriptions/view.js

					
				@ -13,0 +23,4 @@

				  }

				  buildSections = () => {

				    const { suggested, claimSearchUris } = this.props;

akinwale (Migrated from github.com) commented

2019-07-26 08:50:20 +02:00

There is currently a sdk bug where if I try to search for claims using channel for claim_type, and having more than 2 items in any_tags, the request times out and no results are returned.

Working with the limitation of 2 items, I decided to display suggested channels based on two random tags from the list of the user's followed tags. This also makes the suggested subscriptions list look dynamic every time a sees the suggestions.

There is currently a sdk bug where if I try to search for claims using `channel` for `claim_type`, and having more than 2 items in `any_tags`, the request times out and no results are returned. Working with the limitation of 2 items, I decided to display suggested channels based on two random tags from the list of the user's followed tags. This also makes the suggested subscriptions list look dynamic every time a sees the suggestions.

akinwale (Migrated from github.com) reviewed 2019-07-26 08:52:59 +02:00

src/page/discover/view.js

akinwale (Migrated from github.com) commented

2019-07-26 08:52:58 +02:00

I don't even know how I managed this!

akinwale (Migrated from github.com) reviewed 2019-07-26 09:00:21 +02:00

src/page/subscriptions/view.js

akinwale (Migrated from github.com) commented

2019-07-26 09:00:21 +02:00

I'm using the entire object at that index here, not just the name value. I would expect the index to always be at the same spot since it is defined as a constant.

I'm using the entire object at that index here, not just the `name` value. I would expect the index to always be at the same spot since it is defined as a constant.