Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support channel definitions from /file/list_homepage #1267

Merged
merged 1 commit into from May 24, 2018

Conversation

daovist
Copy link
Contributor

@daovist daovist commented Apr 3, 2018

This PR for issue #1049

Is ready to be reviewed by @liamcardenas

Uses existing reducers, actions, selectors

Moves component/common/category-list.jsx to its own top level component at component/categoryList because it needs props to fetchChannel and select claimsByChannel and claimsById

Fetches channels in componentDidMount when the category is a channel

Returns false in isCardVisible when !section -- this fixed the error I was getting; I'm not sure exactly how the code for the UI works

Adds const channel to the CategoryList render function, which is an empty array until the data comes in, and once the data comes in is an array of proper uris made with 'buildURIfromcategory` selector data

Maps the channel array to <FileCard> components when appropriate

Displays a ' for a channel (categoryLink, names=[]) when featuredUris[category].lengthis false inDiscoverPage`

Imports CategoryList from new location in Discover and Subscriptions pages

Fixes various linting errors I found along the way

Includes fake data for demo/testing purposes to be removed after review/updates

@lbryio lbryio deleted a comment Apr 6, 2018
@tzarebczan tzarebczan assigned neb-b and unassigned liamcardenas May 10, 2018
@neb-b neb-b self-requested a review May 11, 2018 01:55
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b May 11, 2018
Copy link

@neb-b neb-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really close. I think some of the logic can be removed since we already have code that handles this. It just uses the same data that page 1 of the channel page uses.

key={category}
category={category.split('#')[0]}
categoryLink={category}
names={[]}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prop should just have a default value of []

@@ -1,7 +1,7 @@
// @flow
import React from 'react';
import Page from 'component/page';
import CategoryList from 'component/common/category-list';
import CategoryList from 'component/categoryList';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes aren't needed anymore since subscriptions page isn't using this component

const { canScrollNext, canScrollPrevious } = this.state;
const channel =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better name for this is probably channelURIs

const { canScrollNext, canScrollPrevious } = this.state;
const channel =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also be able to use makeSelectClaimsInChannelForCurrentPage, which is what the channel page uses. Just get the first page of content and you won't have to worry about this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried using in the makeSelectClaimsInChannelForCurrentPage selector in place of the other two and the logic that goes with them, and I keep running into problems. I'm passing props.categoryLink as the uri, which I think should be fine based on console logging props. I have tried various forms of the makeSelect pattern I found in the RewardLink container-component to no avail, including rewriting the selectors, but there are so many not-working version I'm not sure what to push/show you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -109,6 +117,9 @@ class CategoryList extends React.PureComponent<Props, State> {

// check if a card is fully visible horizontally
isCardVisible = (section: HTMLElement) => {
if (!section) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -32,6 +35,11 @@ class CategoryList extends React.PureComponent<Props, State> {
}

componentDidMount() {
const { category, fetchChannel } = this.props;
if (category[0] === '@') {
fetchChannel(category);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use makeSelectFetchingChannelClaims to make sure we aren't already fetching for this channel.


type Props = {
category: string,
names: Array<string>,
categoryLink?: string,
claimsByChannel: {},
claimsById: {},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use the Caim type that I recently created.

@daovist
Copy link
Contributor Author

daovist commented May 22, 2018

This PR still contains sample data to demonstrate the functionality. I will remove it and clean up commit history following approval.

@neb-b
Copy link

neb-b commented May 24, 2018

Lets merge this. @daovist once you remove the mock data I'll merge

@daovist daovist force-pushed the explore-channels branch 2 times, most recently from f576330 to 4f7a647 Compare May 24, 2018 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants