Publishing Take 2 #346

2017-07-10T17:18:31+02:00

6ea86b96 commented

2017-07-10 17:18:31 +02:00

(Migrated from github.com)

No description provided.

kauffj (Migrated from github.com) requested changes 2017-07-10 23:24:57 +02:00

kauffj (Migrated from github.com) left a comment

I ran out of time on review and testing this for today, but here's that set of feedback.

ui/js/actions/content.js

kauffj (Migrated from github.com) commented

2017-07-10 22:41:35 +02:00

Shouldn't this be reject, at least?

ui/js/actions/file_info.js

kauffj (Migrated from github.com) commented

2017-07-10 22:46:51 +02:00

Is there an alternative queuing pattern that could be used here? Or could we simply check if fetch claims is pending via a selector?

ui/js/component/common.js

kauffj (Migrated from github.com) commented

2017-07-10 23:00:15 +02:00

Probably merits its own component?

ui/js/component/form.js

kauffj (Migrated from github.com) commented

2017-07-10 23:01:48 +02:00

I don't think this needs to change now, and I know this wasn't your work, but this is a pattern worth reconsidering before continuing it further.

ui/js/component/publishForm/internal/channelSection.jsx

kauffj (Migrated from github.com) commented

2017-07-10 23:06:46 +02:00

Are these binds if necessary? The () => { } syntax automatically keeps the this context.

Are these binds if necessary? The `() => { }` syntax automatically keeps the `this` context.

ui/js/component/publishForm/internal/channelSection.jsx

					
				@ -0,0 +17,4 @@

				  handleChannelChange(event) {

				    const channel = event.target.value;

				    if (channel === "new") this.setState({ addingChannel: true });

kauffj (Migrated from github.com) commented

2017-07-10 22:49:49 +02:00

Missing braces (mildly surprised this parses)

ui/js/component/publishForm/internal/channelSection.jsx

					
				@ -0,0 +53,4 @@

				  }

				  handleCreateChannelClick(event) {

				    if (this.state.newChannelName.length < 5) {

kauffj (Migrated from github.com) commented

2017-07-10 23:05:52 +02:00

I'm inclined to drop this restriction (https://github.com/lbryio/lbry/issues/769). But I suppose we ought to keep it for now, especially since the error the daemon gives is unclear.

ui/js/component/publishForm/view.jsx

kauffj (Migrated from github.com) commented

2017-07-10 23:19:26 +02:00

The Notice component was only used here and the style does not match the application. I dropped it.

The `Notice` component was only used here and the style does not match the application. I dropped it.

ui/js/constants/action_types.js

					
				@ -63,7 +63,16 @@ export const FETCH_AVAILABILITY_STARTED = "FETCH_AVAILABILITY_STARTED";

				export const FETCH_AVAILABILITY_COMPLETED = "FETCH_AVAILABILITY_COMPLETED";

kauffj (Migrated from github.com) commented

2017-07-10 22:43:53 +02:00

I would propose that we reserve _COMPLETED for async actions that fire regardless of success or failure, and use _SUCCESS instead for actions that only fire when the async event is successful.

I would propose that we reserve `_COMPLETED` for async actions that fire regardless of success or failure, and use `_SUCCESS` instead for actions that only fire when the async event is successful.

ui/js/page/filePage/view.jsx

kauffj (Migrated from github.com) commented

2017-07-10 23:22:29 +02:00

The dual view button needs to be added to this list.

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 07:58:04 +02:00

ui/js/component/publishForm/internal/channelSection.jsx

					
				@ -0,0 +17,4 @@

				  handleChannelChange(event) {

				    const channel = event.target.value;

				    if (channel === "new") this.setState({ addingChannel: true });

6ea86b96 (Migrated from github.com) commented

2017-07-11 07:58:03 +02:00

Braces aren't required if there's only one statement. So

if (something) ...
else if (somethingElse) ...
else ...

is valid

Braces aren't required if there's only one statement. So ``` if (something) ... else if (somethingElse) ... else ... ``` is valid

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 07:58:24 +02:00

ui/js/component/publishForm/internal/channelSection.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-11 07:58:24 +02:00

They are not necessary :)

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 07:58:42 +02:00

ui/js/constants/action_types.js

					
				@ -63,7 +63,16 @@ export const FETCH_AVAILABILITY_STARTED = "FETCH_AVAILABILITY_STARTED";

				export const FETCH_AVAILABILITY_COMPLETED = "FETCH_AVAILABILITY_COMPLETED";

6ea86b96 (Migrated from github.com) commented

2017-07-11 07:58:42 +02:00

Good call, I like it.

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 08:00:10 +02:00

ui/js/actions/content.js

6ea86b96 (Migrated from github.com) commented

2017-07-11 08:00:10 +02:00

oops, yes it should.

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 08:19:19 +02:00

ui/js/constants/action_types.js

					
				@ -63,7 +63,16 @@ export const FETCH_AVAILABILITY_STARTED = "FETCH_AVAILABILITY_STARTED";

				export const FETCH_AVAILABILITY_COMPLETED = "FETCH_AVAILABILITY_COMPLETED";

6ea86b96 (Migrated from github.com) commented

2017-07-11 08:19:19 +02:00

maybe _SUCCEEDED

maybe `_SUCCEEDED`

6ea86b96 (Migrated from github.com) reviewed 2017-07-11 10:14:34 +02:00

ui/js/page/filePage/view.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-11 10:14:34 +02:00

dual view button?

6ea86b96 commented

2017-07-11 12:47:44 +02:00

(Migrated from github.com)

Sorry @kauffj, I accidentally force pushed over a couple of changes you made :(

kauffj (Migrated from github.com) reviewed 2017-07-11 13:48:36 +02:00

ui/js/page/filePage/view.jsx

kauffj (Migrated from github.com) commented

2017-07-11 13:48:36 +02:00

The right most button that shows both editor and preview.

kauffj commented

2017-07-11 13:49:04 +02:00

(Migrated from github.com)

@6ea86b96 np re: force push

6ea86b96 (Migrated from github.com) reviewed 2017-07-12 09:06:08 +02:00

ui/js/component/publishForm/view.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-12 09:06:08 +02:00

ok, dropped.

6ea86b96 (Migrated from github.com) reviewed 2017-07-12 09:16:04 +02:00

ui/js/page/filePage/view.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-12 09:16:04 +02:00

I don't see that in the list of possible types to disallow.

I don't see that in the list of possible types to disallow. ![2017-07-12 at 14 14](https://user-images.githubusercontent.com/20863631/28106079-97579768-670c-11e7-9af8-4fa65b2acf88.jpg)

6ea86b96 commented

2017-07-12 09:20:31 +02:00

(Migrated from github.com)

I've added some extra code to disable the submit button if the bid is not higher than the current claim @kauffj

6ea86b96 (Migrated from github.com) reviewed 2017-07-12 09:24:12 +02:00

ui/js/page/filePage/view.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-12 09:24:12 +02:00

However, I have now noticed that the dual view mode in the publish form is horribly broken.

6ea86b96 (Migrated from github.com) reviewed 2017-07-12 09:29:12 +02:00

ui/js/page/filePage/view.jsx

6ea86b96 (Migrated from github.com) commented

2017-07-12 09:29:12 +02:00

For now I've just disabled side-by-side mode.

For now I've just disabled `side-by-side` mode.

6ea86b96 (Migrated from github.com) reviewed 2017-07-12 09:29:52 +02:00

ui/js/component/form.js

6ea86b96 (Migrated from github.com) commented

2017-07-12 09:29:52 +02:00

Definitely need to think about a better solution.

6ea86b96 commented

2017-07-12 10:27:48 +02:00

(Migrated from github.com)

Found and fixed one other bug where if you overwrite an existing claim the old fileinfo shows up in downloaded files.

6ea86b96 commented

2017-07-12 10:40:00 +02:00

(Migrated from github.com)

One more bug I found but have not fixed though.

publish a claim
update that claim
delete the claim
now the fileinfo from the first publish is displaying in the downloaded files

This is because even though the claim was abandoned the daemon is still returning the first file from the file_list command (but not returning the claim anymore in claim_list_mine).

One more bug I found but have not fixed though. - publish a claim - update that claim - delete the claim - now the fileinfo from the first publish is displaying in the downloaded files This is because even though the claim was abandoned the daemon is still returning the first file from the `file_list` command (but not returning the claim anymore in `claim_list_mine`).

Sign in to join this conversation.

No reviewers

kauffj

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#346

				`@ -63,7 +63,16 @@ export const FETCH_AVAILABILITY_STARTED = "FETCH_AVAILABILITY_STARTED";`
				`export const FETCH_AVAILABILITY_COMPLETED = "FETCH_AVAILABILITY_COMPLETED";`