Issue/7152 #7214

Closed
billycolon wants to merge 9 commits from issue/7152 into master
4 changed files with 18 additions and 13 deletions
Showing only changes of commit 5dfc247685 - Show all commits

View file

@ -2,13 +2,25 @@ import { connect } from 'react-redux';
import DownloadProgress from './view'; import DownloadProgress from './view';
import { doSetPlayingUri, doStopDownload, doUpdateDownloadingStatus } from 'redux/actions/content'; import { doSetPlayingUri, doStopDownload, doUpdateDownloadingStatus } from 'redux/actions/content';
import { selectFileInfosByOutpoint } from 'lbry-redux'; import { selectFileInfosByOutpoint } from 'lbry-redux';
import { selectPrimaryUri, selectPlayingUri } from 'redux/selectors/content';
const select = (state) => { const select = (state) => {
const byOutpoint = selectFileInfosByOutpoint(state); const byOutpoint = selectFileInfosByOutpoint(state);
const runningByOutpoint = []; const runningByOutpoint = [];
jessopb commented 2021-10-01 22:58:12 +02:00 (Migrated from github.com)
Review

I think I'd prefer to use the selector for consistency
4c72a563da/ui/redux/selectors/file_info.js (L6)

return { downloadList: selectFileInfosByOutpoint(state) }

I think I'd prefer to use the selector for consistency https://github.com/lbryio/lbry-desktop/blob/4c72a563daf15502787bbada67958ab72f63db59/ui/redux/selectors/file_info.js#L6 return { downloadList: selectFileInfosByOutpoint(state) }
const primaryUri = selectPrimaryUri(state);
const playingUri = selectPlayingUri(state);
const uri = playingUri ? playingUri.uri : null;
for (const key in byOutpoint) { for (const key in byOutpoint) {
if (byOutpoint[key] && byOutpoint[key].status === 'running') runningByOutpoint.push(byOutpoint[key]); const item = byOutpoint[key];
if (item && item.status === 'running') {
if (
(!primaryUri || !primaryUri.includes(`/${item.claim_name}`)) &&
(!uri || !uri.includes(`/${item.claim_name}`))
) {
runningByOutpoint.push(item);
}
}
} }
return { return {

View file

@ -154,7 +154,7 @@ function DownloadProgressItem({
jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment
jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment
}; };
return ( return (
<div className=" download-progress__state-container"> <div className="download-progress__state-container">
jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment
jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment
<div className="download-progress__state-bar"> <div className="download-progress__state-bar">
<Button <Button
label={title} label={title}

jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment
jessopb commented 2021-10-01 23:15:06 +02:00 (Migrated from github.com)
Review

(Object.keys(props.downloadList): any).map(..)
Will fix your inevitable flowtype complaint here :)

(Object.keys(props.downloadList): any).map(..) Will fix your inevitable flowtype complaint here :)
jessopb commented 2021-10-01 23:22:28 +02:00 (Migrated from github.com)
Review

does our Button component work here?

        <Button
          iconSize={24}
          button="close" <- or your className?
          aria-label={__('Close')}
          icon={ICONS.REMOVE}
          onClick={...}
        />
        ```
does our Button component work here? ``` <Button iconSize={24} button="close" <- or your className? aria-label={__('Close')} icon={ICONS.REMOVE} onClick={...} /> ```
jessopb commented 2021-10-01 23:24:46 +02:00 (Migrated from github.com)
Review

All strings can be wrapped in __('some text') for i18n

All strings can be wrapped in __('some text') for i18n
jessopb commented 2021-10-01 23:51:27 +02:00 (Migrated from github.com)
Review

This could be <Button button="link" label={__('Yes')} /> if it works well.

This could be <Button button="link" label={__('Yes')} /> if it works well.
jessopb commented 2021-10-02 01:42:48 +02:00 (Migrated from github.com)
Review

This {title} should probably link to the FilePage for the content url
Since an item in downloadList includes a claim_id and a claim_name, you can try using something like

<Button
...
navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id }))
>
This {title} should probably link to the FilePage for the content url Since an item in downloadList includes a claim_id and a claim_name, you can try using something like ``` <Button ... navigate={buildURI({ claimName: item.claim_name, claimID: item.claim_id })) > ```
jessopb commented 2021-10-02 02:09:01 +02:00 (Migrated from github.com)
Review

Neat to see this done from scratch and in about the same way!
We have this util function:
https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js

Neat to see this done from scratch and in about the same way! We have this util function: https://github.com/lbryio/lbry-desktop/blob/master/ui/util/format-bytes.js
jessopb commented 2021-10-02 06:00:24 +02:00 (Migrated from github.com)
Review

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.

I think DownloadProgressItem be clearer? The word "state" takes a second for me to disambiguate.
jessopb commented 2021-10-02 07:14:36 +02:00 (Migrated from github.com)
Review

we can persist this using our usePersistedState() hook

we can persist this using our usePersistedState() hook
jessopb commented 2021-10-02 08:03:23 +02:00 (Migrated from github.com)
Review

something weird happens here that crashes the app when I play a video.
"cannot read status of null"
redux devtools (which has a bug that sometimes doesn't load unless you yarn dev and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { : null } until file_list fetch comes back.

something weird happens here that crashes the app when I play a video. "cannot read status of null" redux devtools (which has a bug that sometimes doesn't load unless you `yarn dev` and wait for the app to finish before opening devtools) tells me initial state byOutpoint is { <outpoint>: null } until file_list fetch comes back.
jessopb commented 2021-10-08 20:03:24 +02:00 (Migrated from github.com)
Review

this name sounds like a function rather than an array.
downloadsToUpdate?

this name sounds like a function rather than an array. downloadsToUpdate?
jessopb commented 2021-10-08 20:07:16 +02:00 (Migrated from github.com)
Review

this could use a comment

this could use a comment

View file

@ -73,17 +73,9 @@ function FileDownloadLink(props: Props) {
if (fileInfo && fileInfo.written_bytes > 0) { if (fileInfo && fileInfo.written_bytes > 0) {
const progress = (fileInfo.written_bytes / fileInfo.total_bytes) * 100; const progress = (fileInfo.written_bytes / fileInfo.total_bytes) * 100;
return ( return <span className="download-text">{__('%percent%% downloaded', { percent: progress.toFixed(0) })}</span>;
<span className="download-text" style={{ backgroundColor: '#FFF' }}>
{__('%percent%% downloaded', { percent: progress.toFixed(0) })}
</span>
);
} else { } else {
return ( return <span className="download-text">{__('Connecting...')}</span>;
<span className="download-text" style={{ backgroundColor: '#FFF' }}>
{__('Connecting...')}
</span>
);
} }
} }
// @endif // @endif

View file

@ -720,6 +720,7 @@
margin: 0 0; margin: 0 0;
padding: var(--spacing-xxs) var(--spacing-xxs); padding: var(--spacing-xxs) var(--spacing-xxs);
height: unset; height: unset;
background-color: var(--color-header-background);
// label (with 'Add' text) hidden by default // label (with 'Add' text) hidden by default
.button__label { .button__label {