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

Select thumbnail, spee.ch upload #1248

Merged
merged 25 commits into from Jun 8, 2018
Merged

Conversation

daovist
Copy link
Contributor

@daovist daovist commented Apr 2, 2018

This PR...

Lets <FileSelector> take custom labels

Adds thumbnail_upload_statuses constants

Adds <SelectThumbnail> component and uses it in <PublishForm>

Adds ModalConfirmThumbnailUpload which is called by <SelectThumbnail>

Adds uploadThumbnailStatus to PublishState (and not PublishParams) in publishReducer

Uses existing reducers and selectors, adds doResetThumbnailStatus and doUploadThumbnail actions for spee.ch requests

Adds a.link CSS

Cleans up some errors I got committing changes

@daovist
Copy link
Contributor Author

daovist commented Apr 10, 2018

@liamcardenas this is ready for review. I think I got the rebase right but I'm not positive. I tested it post merge, commit, push, and it works fine.

A couple things that could/maybe should be done:

The modal and publish form write to the same state.publish.nsfw so if the thumbnail is marked nsfw it will be checked when the user gets to that option on the page. Should we turn off the ability to not mark a file nsfw when the thumbnail being uploaded was marked nsfw, by disabling the checkbox?

Should we disable publishing until the upload is complete? Until it does, the thumbnail field is a blank string, it doesn't get back the url with a claim_id until the upload is complete.

@tzarebczan tzarebczan requested review from neb-b and removed request for liamcardenas May 10, 2018 01:26
@daovist daovist force-pushed the select-thumbnail branch 2 times, most recently from ee8b1c4 to 8fea5ec Compare May 21, 2018 14:38
@daovist
Copy link
Contributor Author

daovist commented May 21, 2018

@seanyesmunt this is working and ready for review

Copy link
Contributor

@skhameneh skhameneh left a comment

Choose a reason for hiding this comment

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

Approved w/ minor comments

const { type, currentPath, fileLabel, directoryLabel } = this.props;

const label =
type === 'file' ? fileLabel || __('Choose File') : directoryLabel || __('Choose Directory');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add parens for legibility?

@@ -73,6 +77,10 @@ class PublishForm extends React.PureComponent<Props> {
(this: any).getNewUri = this.getNewUri.bind(this);
}

componentWillMount() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is being deprecated, it's good to start using getDerivedStateFromProps()

https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops

const makeid = () => {
let text = '';
const possible = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
for (let i = 0; i < 24; i += 1) text += possible.charAt(Math.floor(Math.random() * 62));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split this line out with brackets for legibility?

const data = new FormData();
const name = makeid();
const blob = new Blob([thumbnail], { type: `image/${fileExt.slice(1)}` });
data.append('name', name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clean up some of the formatting from ln90 to 110?
Line breaks, indentation, etc

@daovist
Copy link
Contributor Author

daovist commented May 31, 2018

I cleaned this up based on comments from @skhameneh and other changes since the last time I worked on select-thumbnail. I moved constants to lbry-redux in /lbryio/lbry-redux/pull/39 which is a prereq for this PR.

I did not implement getDerivedStateFromProps thinking that pattern is all over the app and should be its own issue to clean up everywhere.

It now resets the thumbnail upload status when the whole form is reset. I refactored the errors and tested it on a faulty URL and everything worked like it should. Though some more @tzarebczan testing is probably in order.

export const SEND_TIP = 'send_tip';
export const PUBLISH = 'publish';
export const SEARCH = 'search';
export const CONFIRM_THUMBNAIL_UPLOAD = 'confirmThumbnailUpload';
Copy link

Choose a reason for hiding this comment

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

This should be in lbry-redux

@@ -30,6 +36,85 @@ export const doUpdatePublishForm = (publishFormValue: UpdatePublishFormData) =>
data: { ...publishFormValue },
});

export const doResetThumbnailStatus = () => (dispatch: Dispatch): Action =>
fetch('https://spee.ch/api/channel/availability/@testing')
Copy link

Choose a reason for hiding this comment

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

Is this the correct path?

@neb-b neb-b merged commit 65513cf into lbryio:master Jun 8, 2018
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