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

Discovery UI #2477

Merged
merged 12 commits into from
Jun 19, 2019
Merged

Discovery UI #2477

merged 12 commits into from
Jun 19, 2019

Conversation

NetOpWibby
Copy link
Contributor

@NetOpWibby NetOpWibby commented May 10, 2019

Changes

  • Added new trending homepage with tag follower interface
  • Consistency improvements
    • FileCard, FileTile, and ChannelTile are merged into one component FileListItem
    • Got rid of a decent amount of associated styles

Notes

  • This is not mergeable. But it's in a state where it can begin to be reviewed. I would ignore the .scss files for now. I need to make a PR to github.com/lbryio/color before the styles are complete.
  • There are a number of UI issues that I am aware of (mostly overlap issues, things not wrapping). I just want to get some eyes on the core changes while I finish up the rest of the small fixes.
  • Depends on Remove unused style and allow input borders to be changed with less code components#10

@kauffj
Copy link
Member

kauffj commented May 15, 2019

Attempted to run the discover branch and encountered errors using the UI. I'm going to do a code review, but I'm unable to do a UI/UX review.

Screenshot from 2019-05-15 10-37-12

src/ui/component/discoveryFirstRun/view.jsx Outdated Show resolved Hide resolved
src/ui/redux/actions/tags.js Outdated Show resolved Hide resolved
src/ui/redux/reducers/tags.js Outdated Show resolved Hide resolved
src/ui/scss/component/_discover.scss Outdated Show resolved Hide resolved
@neb-b
Copy link

neb-b commented May 15, 2019

I will be making a number of changes to this branch today as I wire up redux.

@lbry-bot lbry-bot assigned neb-b and unassigned neb-b May 15, 2019
@neb-b neb-b force-pushed the discovery branch 2 times, most recently from 3036074 to 6064bef Compare May 23, 2019 17:45
@@ -12,6 +12,7 @@
<meta property="og:description" content="All your favorite LBRY content in your browser." />
<meta property="og:image" content="/og.png" />
<!-- @endif -->
<meta name="viewport" content="width=device-width, initial-scale=1" />
Copy link

Choose a reason for hiding this comment

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

@kauffj You said not to build a mobile layout, but you didn't say not to create a new desktop layout that just happens to look good on mobile too.

src/ui/page/discover/view.jsx Outdated Show resolved Hide resolved
) : (
header
)}
<div className="file-list__sort">{sort}</div>
Copy link

Choose a reason for hiding this comment

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

Not really a fan of sort. This is the item on the right side of the header. Currently the two pages that use this are the homepage (trending/best/new) and search feedback (satisfied with results?)

Sort isn't really a good name.

Copy link
Member

Choose a reason for hiding this comment

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

headerAltControls? headerSecondaryControls? or same names without the word header?

Copy link
Member

@kauffj kauffj left a comment

Choose a reason for hiding this comment

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

I'm not done but I have to go home.

package.json Outdated Show resolved Hide resolved
src/ui/component/app/view.jsx Outdated Show resolved Hide resolved
src/ui/component/button/view.jsx Outdated Show resolved Hide resolved
src/ui/component/fileList/view.jsx Outdated Show resolved Hide resolved
src/ui/component/fileList/view.jsx Outdated Show resolved Hide resolved
src/ui/component/tagsSearch/view.jsx Show resolved Hide resolved
src/ui/component/tagsSelect/view.jsx Outdated Show resolved Hide resolved
src/ui/component/walletSend/view.jsx Outdated Show resolved Hide resolved
src/ui/scss/component/_file-list.scss Outdated Show resolved Hide resolved
Copy link
Member

@kauffj kauffj left a comment

Choose a reason for hiding this comment

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

mostly done

src/ui/component/fileList/view.jsx Outdated Show resolved Hide resolved
) : (
header
)}
<div className="file-list__sort">{sort}</div>
Copy link
Member

Choose a reason for hiding this comment

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

headerAltControls? headerSecondaryControls? or same names without the word header?

src/ui/component/fileListTrending/index.js Outdated Show resolved Hide resolved
uris={trending}
injectedItem={personalSort === TRENDING_SORT_YOU && injectedItem}
header={
<React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

Should this just be a component? Or at least not inlined? Same for the sort parameter below

Copy link

Choose a reason for hiding this comment

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

👍 I pulled it out. Possibly there should be FileListHeader component, but I think it helps to keep this logic here, since all of this is related.

src/ui/component/fileListTrending/view.jsx Outdated Show resolved Hide resolved
<FileListTrending
personal
tags={followedTags.map(tag => tag.name)}
injectedItem={<TagsSelect showClose title={__('Make This Your Own')} />}
Copy link
Member

Choose a reason for hiding this comment

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

Mildly uncertain about the pattern of relying on the local persisted state of <TagsSelect> being closed to not re-render this. I suppose that is fine though.

Copy link

Choose a reason for hiding this comment

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

Yeah I wasn't really sure about this. Another option is to always show the injected item. It could be in the third/fourth position if there are more than four items, otherwise, just show at the end.

I figured we would have another on the subscriptions page somewhere.

src/ui/page/fileListDownloaded/view.jsx Outdated Show resolved Hide resolved
const hasDownloads = !!downloadedUris.length;
console.log('downloadedUris', downloadedUris);
return (
// Removed the <Page> wapper to try combining this page with UserHistory
Copy link
Member

Choose a reason for hiding this comment

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

Per comments in Slack, I'm fine with dropping the history, at least temporarily, if that makes this easier/cleaner

Copy link

Choose a reason for hiding this comment

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

It would, and I'm fine with that for now. I'll remove it/make the other changes in the next PR

src/ui/page/subscriptions/view.jsx Outdated Show resolved Hide resolved
location: { search },
} = props;

const urlParams = new URLSearchParams(search);
Copy link
Member

Choose a reason for hiding this comment

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

TIL

@neb-b neb-b merged commit 3af8fa8 into master Jun 19, 2019
@neb-b neb-b deleted the discovery branch June 27, 2019 20:07
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.

3 participants