Subscriptions #811

2017-12-05T01:29:55+01:00

neb-b commented

2017-12-05 01:29:55 +01:00

(Migrated from github.com)

There are a few notable issues (mostly dealing with the FeaturedCategory component), and it could be presented much nicer as subscriptions are being loaded. I think we should try to get this in now, to try and catch any major bugs. I will continue to clean it up and improve the styling over the next couple days and fix any major bugs we find during RC testing.

Notable issues:
~~Subscriptions don't load when refreshing the page~~
(hacked together a fix, we will need to rework the initial load to wait for the rehydrate action)

Can't subscribe to pages with no content
Can only view the first 10 files per channel on the subscriptions page (this should be easy to fix)

The actual saving of subscriptions seems to work pretty well though

There are a few notable issues (mostly dealing with the `FeaturedCategory` component), and it could be presented much nicer as subscriptions are being loaded. I think we should try to get this in now, to try and catch any major bugs. I will continue to clean it up and improve the styling over the next couple days and fix any major bugs we find during RC testing. Notable issues: ~Subscriptions don't load when refreshing the page~ (hacked together a fix, we will need to rework the initial load to wait for the rehydrate action) Can't subscribe to pages with no content Can only view the first 10 files per channel on the subscriptions page (this should be easy to fix) The actual saving of subscriptions seems to work pretty well though

kauffj (Migrated from github.com) reviewed 2017-12-05 01:29:55 +01:00

neb-b (Migrated from github.com) reviewed 2017-12-08 00:40:37 +01:00

src/renderer/js/redux/reducers/subscriptions.js

neb-b (Migrated from github.com) commented

2017-12-08 00:40:37 +01:00

I wasn't seeing Flow catch state errors when I moved all of the types into types/subscriptions.js, but it is working if kept in the reducer file.

Needs more exploring.

I wasn't seeing `Flow` catch state errors when I moved all of the types into `types/subscriptions.js`, but it is working if kept in the reducer file. Needs more exploring.

liamcardenas (Migrated from github.com) reviewed 2017-12-08 02:18:41 +01:00

src/renderer/js/redux/reducers/subscriptions.js

liamcardenas (Migrated from github.com) commented

2017-12-08 02:18:41 +01:00

interesting :/

liamcardenas (Migrated from github.com) reviewed 2017-12-08 02:23:19 +01:00

src/renderer/js/page/file/view.jsx

liamcardenas (Migrated from github.com) commented

2017-12-08 02:23:19 +01:00

what are the cases in which this would be false?

neb-b (Migrated from github.com) reviewed 2017-12-08 02:25:55 +01:00

src/renderer/js/page/file/view.jsx

neb-b (Migrated from github.com) commented

2017-12-08 02:25:55 +01:00

If someone posts an anonymous video. It might be fine with just the !!channel check. If there is no channelName there will be no channelClaimId (i think?)

If someone posts an anonymous video. It might be fine with just the `!!channel` check. If there is no `channelName` there will be no `channelClaimId` (i think?)

liamcardenas (Migrated from github.com) reviewed 2017-12-08 02:30:45 +01:00

src/renderer/js/redux/actions/subscriptions.js

liamcardenas (Migrated from github.com) commented

2017-12-08 02:25:33 +01:00

no flow on this page?

neb-b (Migrated from github.com) reviewed 2017-12-08 02:31:56 +01:00

src/renderer/js/redux/actions/subscriptions.js

neb-b (Migrated from github.com) commented

2017-12-08 02:31:56 +01:00

whoops. I added the types, but just forgot to re-add the flow comment

neb-b (Migrated from github.com) reviewed 2017-12-08 04:32:17 +01:00

src/renderer/js/page/file/view.jsx

neb-b (Migrated from github.com) commented

2017-12-08 04:32:17 +01:00

@kauffj any problems with only checking for channelName?

@kauffj any problems with only checking for `channelName`?

liamcardenas (Migrated from github.com) reviewed 2017-12-08 17:42:01 +01:00

src/renderer/js/page/file/view.jsx

liamcardenas (Migrated from github.com) commented

2017-12-08 17:42:00 +01:00

FWIW, that is what I would do ^

liamcardenas commented

2017-12-08 17:48:58 +01:00

(Migrated from github.com)

small nitpick, does anyone else agree that this spacing is too much between the Home / Subscriptions menu and Trending?
screen shot 2017-12-08 at 8 48 17 am

small nitpick, does anyone else agree that this spacing is too much between the Home / Subscriptions menu and Trending? <img width="297" alt="screen shot 2017-12-08 at 8 48 17 am" src="https://user-images.githubusercontent.com/5027235/33775881-994db7ac-dbf4-11e7-9a21-252df2997bcd.png">

liamcardenas (Migrated from github.com) approved these changes 2017-12-08 17:55:07 +01:00

liamcardenas (Migrated from github.com) left a comment

I checked all the scenarios I could think of-- everything worked beautifully. I think maybe there could be some discussion around the optimal subscription button placement within the file page but for now this is good.

I left one comment about styling that we can talk about / fix if possible. Otherwise, nbd.

Approving...

I checked all the scenarios I could think of-- everything worked beautifully. I think maybe there could be some discussion around the optimal subscription button placement within the file page but for now this is good. I left one comment about styling that we can talk about / fix if possible. Otherwise, nbd. Approving...

neb-b commented

2017-12-08 18:40:03 +01:00

(Migrated from github.com)

@liamcardenas agreed. dropped it down to 24px

👍 1

neb-b (Migrated from github.com) reviewed 2017-12-08 19:15:49 +01:00

src/renderer/js/component/subHeader/view.jsx

neb-b (Migrated from github.com) commented

2017-12-08 19:15:49 +01:00

the modifier prop wasn't being used anywhere

the `modifier` prop wasn't being used anywhere

kauffj (Migrated from github.com) reviewed 2017-12-08 20:11:30 +01:00

kauffj (Migrated from github.com) left a comment

Mostly small stuff

src/renderer/js/component/header/view.jsx

kauffj (Migrated from github.com) commented

2017-12-08 16:35:01 +01:00

I'm torn as to whether this should be two separate buttons in the header, or one button and the "Home" page should remember your preference between Discover and Subscriptions. Inclined to think the latter is better.

src/renderer/js/page/channel/view.jsx

kauffj (Migrated from github.com) commented

2017-12-08 19:50:56 +01:00

Wrap these labels in __

Wrap these labels in `__`

src/renderer/js/page/channel/view.jsx

kauffj (Migrated from github.com) commented

2017-12-08 19:56:33 +01:00

When this button is labeled "Subscribe", let's give it the same icon used elsewhere to indicate subscriptions.

When it is labeled unsubscribe, make it alt rather than primary.

Also, what do you think about including this in <FileActions>?

When this button is labeled "Subscribe", let's give it the same icon used elsewhere to indicate subscriptions. When it is labeled unsubscribe, make it `alt` rather than primary. Also, what do you think about including this in `<FileActions>`?

src/renderer/js/page/file/view.jsx

kauffj (Migrated from github.com) commented

2017-12-08 19:58:27 +01:00

The repetition of this code across ChannelPage and FilePage looks like a pretty significant DRY violation.

Any reason this can't just be <SubscriptionButton uri={uri} /> and then the rest of the logic lives shared inside of the component?

The repetition of this code across `ChannelPage` and `FilePage` looks like a pretty significant DRY violation. Any reason this can't just be `<SubscriptionButton uri={uri} />` and then the rest of the logic lives shared inside of the component?

src/renderer/js/page/subscriptions/view.jsx

kauffj (Migrated from github.com) commented

2017-12-08 20:01:53 +01:00

I'm okay shipping it like this, but this could be an example where the empty vs. undefined is a useful distinction. If the subscription array starts undefined, then we can differentiate between never fetched (undefined) and fetched but empty (empty array).

kauffj (Migrated from github.com) commented

2017-12-08 20:03:46 +01:00

const someClaimsNotLoaded = Boolean(subscriptions.find(subscription => subscription.claims.length))

`const someClaimsNotLoaded = Boolean(subscriptions.find(subscription => subscription.claims.length))`

kauffj (Migrated from github.com) commented

2017-12-08 20:05:53 +01:00

This could conceivably be moved into the/a selector. I get the impression that most data wrangling should happen inside of Redux rather than React.

src/renderer/js/redux/actions/subscriptions.js

kauffj (Migrated from github.com) commented

2017-12-08 19:50:22 +01:00

Probably should be doChannelSubscribe and doChannelUnsubscribe to match existing conventions

Probably should be `doChannelSubscribe` and `doChannelUnsubscribe` to match existing conventions

src/renderer/js/redux/selectors/navigation.js

kauffj (Migrated from github.com) commented

2017-12-08 20:06:22 +01:00

Let's commit to naming this "Discover"

src/renderer/scss/component/_tabs.scss

					
				@ -51,2 +51,4 @@

				    }

				  }

				  &.sub-header--full-width {

kauffj (Migrated from github.com) commented

2017-12-08 20:11:16 +01:00

I have some non-blocking concerns about this, but will wait to discuss them on a meeting.

src/renderer/scss/component/_tabs.scss

					
				@ -55,3 +63,4 @@

				@keyframes activeTab {

				    from {width: 0;}

				    to {width: 100%;}

kauffj (Migrated from github.com) commented

2017-12-08 20:10:41 +01:00

There appear to be two binary files below that shouldn't have been committed.

neb-b (Migrated from github.com) reviewed 2017-12-08 21:26:55 +01:00

src/renderer/js/page/subscriptions/view.jsx

neb-b (Migrated from github.com) commented

2017-12-08 21:26:55 +01:00

👍 always forget about Array.find

neb-b commented

2017-12-08 22:11:39 +01:00

(Migrated from github.com)

@kauffj We can probably move the subscribe button inside FileActions, I think we should keep it as is for now just to get it in

@kauffj We can probably move the subscribe button inside `FileActions`, I think we should keep it as is for now just to get it in

Sign in to join this conversation.

No reviewers

kauffj

liamcardenas

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#811

Rows
Columns