Moar discovery #2553

2019-06-17T21:41:23+02:00

neb-b commented

2019-06-17 21:41:23 +02:00

(Migrated from github.com)

Building off my last PR

Changes

Added follow/unfollow on the tags page
Added menu buttons in the header to navigate to different pages
Reworked the "Account" page to be the lbry inc hub

Building off my last PR #### Changes - Added follow/unfollow on the tags page - Added menu buttons in the header to navigate to different pages - Reworked the "Account" page to be the lbry inc hub

kauffj (Migrated from github.com) approved these changes 2019-06-18 23:48:45 +02:00

kauffj (Migrated from github.com) left a comment

Most of this feedback is minor. Did not do a thorough UI/UX test, just code.

src/ui/component/fileList/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:15:21 +02:00

header and noHeader could be a single param, unless it's considered a bad practice to say a type can be a Node or boolean (true = default, false = none, node = inject)

`header` and `noHeader` could be a single param, unless it's considered a bad practice to say a type can be a `Node` or `boolean` (true = default, false = none, node = inject)

src/ui/component/fileListItem/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:17:05 +02:00

Same for this, if this is being used on channels and files, call it ClaimTags

Same for this, if this is being used on channels and files, call it `ClaimTags`

src/ui/component/fileListItem/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:16:30 +02:00

channels aren't files

if this component is now rendering streams and channels, I'd rename it ClaimListItem or similar

channels aren't files if this component is now rendering streams and channels, I'd rename it `ClaimListItem` or similar

src/ui/component/fileTags/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:18:37 +02:00

slightly worse pattern, because passing true for both large and slim should not be possible and would have unexpected behavior

src/ui/component/fileTags/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:17:50 +02:00

good pattern!

src/ui/component/header/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:36:06 +02:00

I happened to have my app in Polish while running this branch and noticed this menu looks a little weird when the text of the items is longer than they are in English.

kauffj (Migrated from github.com) commented

2019-06-18 23:37:11 +02:00

I also kind of dislike this from a UX perspective. I'd rather just put light/dark in settings, give Help first-class status, and lose the dropdown entirely if these are the only 3 items in it.

src/ui/component/rewardSummary/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:22:21 +02:00

While I know we've used this approach elsewhere, it's a bad practice and doesn't really work for i18n.

Is there any ability to inject nodes inside of an i18n string? e.g. __('You have %amount% LBC', { amount: <span>5</span> })

If not, let's use a comment tag so we can track where these exist. Something like @i18nfixme.

While I know we've used this approach elsewhere, it's a bad practice and doesn't really work for i18n. Is there any ability to inject nodes inside of an i18n string? e.g. `__('You have %amount% LBC', { amount: <span>5</span> })` If not, let's use a comment tag so we can track where these exist. Something like `@i18nfixme`.

src/ui/component/rewardTotal/view.jsx

					
				@ -0,0 +16,4 @@

				  return (

				    <section className="card  card--section card--reward-total" style={{ backgroundImage: `url(${TotalBackground})` }}>

				      <span className="card__title">

				        {integer} LBC {__('Earned From Rewards')}

kauffj (Migrated from github.com) commented

2019-06-18 23:30:06 +02:00

https://github.com/lbryio/lbry/issues/2245

src/ui/component/userEmail/view.jsx

					
				@ -0,0 +11,4 @@

				  resendVerificationEmail: string => void,

				  checkEmailVerified: () => void,

				  user: {

				    has_verified_email: boolean,

kauffj (Migrated from github.com) commented

2019-06-18 23:32:47 +02:00

Should user be a first class type and/or should this value simply be selected directly off of the user?

src/ui/page/auth/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:39:22 +02:00

looks like a nice refactor!

src/ui/page/file/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:42:52 +02:00

I'm pretty sure I can explain/fix this, but easier to do so synchronously. Please bring up on next standup we're both on.

src/ui/page/invite/view.jsx

kauffj (Migrated from github.com) commented

2019-06-18 23:43:11 +02:00

empty class

src/ui/page/subscriptions/view.jsx

					
				@ -33,0 +55,4 @@

				    const options = {

				      channel_ids: ids,

				    };

kauffj (Migrated from github.com) commented

2019-06-18 23:43:58 +02:00

is the channel uri guaranteed to have a #? they will never be URIs without a claim id?

is the channel uri guaranteed to have a `#`? they will never be URIs without a claim id?

src/ui/scss/component/_file-list.scss

					
				@ -17,0 +28,4 @@

				.file-list__header--small {

				  height: 3rem;

				  font-size: 1em;

kauffj (Migrated from github.com) commented

2019-06-18 23:45:16 +02:00

minor tsk tsk for BEM violation

src/ui/scss/component/_file-properties.scss

					
				@ -8,2 +6,4 @@

				  & > *:not(:last-child) {

				    margin-right: var(--spacing-small);

				  }

kauffj (Migrated from github.com) commented

2019-06-18 23:46:22 +02:00

minor boos

src/ui/util/use-tween.js

					
				@ -0,0 +1,31 @@

				import { useEffect, useState } from 'react';

kauffj (Migrated from github.com) commented

2019-06-18 23:48:07 +02:00

I approve of copy and pasting this wherever you copy and pasted it from

neb-b (Migrated from github.com) reviewed 2019-06-19 04:35:20 +02:00

src/ui/page/subscriptions/view.jsx

					
				@ -33,0 +55,4 @@

				    const options = {

				      channel_ids: ids,

				    };

neb-b (Migrated from github.com) commented

2019-06-19 04:35:20 +02:00

The channel id is required (for now?) in internal-apis for subscriptions. The app always uses the full uri for the subscribeButton component

neb-b (Migrated from github.com) reviewed 2019-06-19 04:36:12 +02:00

src/ui/util/use-tween.js

					
				@ -0,0 +1,31 @@

				import { useEffect, useState } from 'react';

neb-b (Migrated from github.com) commented

2019-06-19 04:36:12 +02:00

📈

neb-b (Migrated from github.com) reviewed 2019-06-19 04:38:45 +02:00

src/ui/component/userEmail/view.jsx

					
				@ -0,0 +11,4 @@

				  resendVerificationEmail: string => void,

				  checkEmailVerified: () => void,

				  user: {

				    has_verified_email: boolean,

neb-b (Migrated from github.com) commented

2019-06-19 04:38:45 +02:00

Yes to both of these

neb-b (Migrated from github.com) reviewed 2019-06-19 04:41:57 +02:00

src/ui/component/fileListItem/view.jsx

neb-b (Migrated from github.com) commented

2019-06-19 04:41:57 +02:00

Nice good call. Will rename all of these to claimX

Nice good call. Will rename all of these to `claimX`

neb-b (Migrated from github.com) reviewed 2019-06-19 05:02:06 +02:00

src/ui/component/fileList/view.jsx

neb-b (Migrated from github.com) commented

2019-06-19 05:02:05 +02:00

No I think that makes more sense. Will update.

neb-b (Migrated from github.com) reviewed 2019-06-19 05:15:27 +02:00

src/ui/component/fileTags/view.jsx

neb-b (Migrated from github.com) commented

2019-06-19 05:15:27 +02:00

Changed to type={"small", "large"}

Changed to `type={"small", "large"}`

neb-b (Migrated from github.com) reviewed 2019-06-19 05:50:58 +02:00

src/ui/component/header/view.jsx

neb-b (Migrated from github.com) commented

2019-06-19 05:50:58 +02:00

I'm fine with moving help out of the menu, but I would really like to keep the dark mode toggle. YouTube, Twitter, and Reddit all have a dark mode toggle in their drop-down menu.