#323 - React/Redux - publish component - LBRYCommunity/spee.ch

You should look at some of the linting additions @IGassman has added to app, could help keep these imports consistent in path specification (among other improvements).

react/reducers/index.js

kauffj (Migrated from github.com) commented

2018-01-15 20:27:22 +01:00

import * as

`import * as`

kauffj (Migrated from github.com) commented

2018-01-15 20:28:40 +01:00

Some of these should possibly be renamed or refactored into separate files (or possibly can wait until next refactor/revision).

react/utils/file.js

kauffj (Migrated from github.com) commented

2018-01-15 20:26:57 +01:00

GIFs

react/utils/page.js

kauffj (Migrated from github.com) commented

2018-01-15 20:26:37 +01:00

Shouldn't this design be obviated by addition of React/Redux? The component should be able to refresh based on external state change

views/index.handlebars

kauffj (Migrated from github.com) commented

2018-01-15 20:35:20 +01:00

This doesn't seem exactly on theme. Maybe ask nizuka?

etisdew (Migrated from github.com) reviewed 2018-01-15 21:16:12 +01:00

react/components/ProgressBar.jsx

etisdew (Migrated from github.com) commented

2018-01-15 21:16:12 +01:00

It looks as if its seeding the progress visually, is that correct?

etisdew (Migrated from github.com) reviewed 2018-01-15 21:38:07 +01:00

react/utils/file.js

etisdew (Migrated from github.com) commented

2018-01-15 21:38:07 +01:00

I'm interested in the direction of this PR, it seems to be purposed as an image sharing service as per required. .png .jpeg .gif ... requirements. Can we still publish video and is there support for generic file types? It just feels like its moving away from video upload and I wanted to be clear.

neb-b (Migrated from github.com) reviewed 2018-01-18 06:08:36 +01:00

react/components/Preview.jsx

neb-b (Migrated from github.com) commented

2018-01-18 06:08:35 +01:00

Generally you should avoid doing dom stuff/setting state in componentWillMount. Especially since it seems that they will be removing it in a future React version.

componentDidMount is preferred

Generally you should avoid doing dom stuff/setting state in `componentWillMount`. Especially since it seems that they will be removing it in a future React version. `componentDidMount` is preferred

neb-b (Migrated from github.com) reviewed 2018-01-18 06:16:05 +01:00

react/components/ProgressBar.jsx

neb-b (Migrated from github.com) commented

2018-01-18 06:16:04 +01:00

Instead of adding react elements to state, just store the data.

Then in the render function, you can do
this.state.bars.map((bar, index) => bar.isActive ? <ActiveBar key={index} /> : <InactiveBar key={index} />)

Instead of adding react elements to state, just store the data. Then in the `render` function, you can do `this.state.bars.map((bar, index) => bar.isActive ? <ActiveBar key={index} /> : <InactiveBar key={index} />)`

neb-b (Migrated from github.com) reviewed 2018-01-18 06:19:30 +01:00

react/containers/ChannelLoginForm/view.jsx

neb-b (Migrated from github.com) commented

2018-01-18 06:19:30 +01:00

Forms are one of the most painful things in React. It might be worth looking into 3rd party React form elements. I've done a fair bit of work with forms and think that Formik is really nice. Hoping to add it to more parts of the app soon. https://github.com/jaredpalmer/formik

I really like how they use the render prop, I think it's a really nice way to interact with React elements

Forms are one of the most painful things in React. It might be worth looking into 3rd party React form elements. I've done a fair bit of work with forms and think that Formik is really nice. Hoping to add it to more parts of the app soon. https://github.com/jaredpalmer/formik I really like how they use the `render` prop, I think it's a really nice way to interact with React elements

neb-b (Migrated from github.com) reviewed 2018-01-18 06:19:56 +01:00

react/containers/ChannelSelect/view.jsx

neb-b (Migrated from github.com) commented

2018-01-18 06:19:56 +01:00

Same here, this should be moved to componentDidMount

Same here, this should be moved to `componentDidMount`

neb-b (Migrated from github.com) reviewed 2018-01-18 06:24:14 +01:00

react/containers/ChannelSelect/view.jsx

neb-b (Migrated from github.com) commented

2018-01-18 06:24:14 +01:00

Instead of reading prop values and adding them to state, you should just use the prop values and not worry about internal state.

I try to avoid internal state whenever possible, one exception is the app homepage, where we store if the user can scroll left or right in a card row. But that isn't dependent on any prop value, just by what is currently on the screen.

Whenever you separate the data into two sources it can cause some weird issues.

Instead of reading prop values and adding them to state, you should just use the prop values and not worry about internal state. I try to avoid internal state whenever possible, one exception is the app homepage, where we store if the user can scroll left or right in a card row. But that isn't dependent on any prop value, just by what is currently on the screen. Whenever you separate the data into two sources it can cause some weird issues.

bones7242 (Migrated from github.com) reviewed 2018-01-18 18:48:36 +01:00

react/utils/file.js

bones7242 (Migrated from github.com) commented

2018-01-18 18:48:35 +01:00

@etisdew yes video/mp4 is supported and supporting video uploads is a key goal for spee.ch

@etisdew yes `video/mp4` is supported and supporting video uploads is a key goal for spee.ch

bones7242 (Migrated from github.com) reviewed 2018-01-18 23:25:06 +01:00

react/utils/page.js

bones7242 (Migrated from github.com) commented

2018-01-18 23:25:06 +01:00

The reason for this was that html outside of the react publish component (the top nav bar) needed to be updated. To solve this, I added a nav bar component that can draw from the store.

bones7242 (Migrated from github.com) reviewed 2018-01-18 23:26:24 +01:00

react/components/ProgressBar.jsx

bones7242 (Migrated from github.com) commented

2018-01-18 23:26:24 +01:00

Yes, I am rendering the |s for the progress bars

Yes, I am rendering the `|`s for the progress bars

bones7242 (Migrated from github.com) reviewed 2018-01-19 00:57:09 +01:00

react/containers/ChannelSelect/view.jsx

bones7242 (Migrated from github.com) commented

2018-01-19 00:57:08 +01:00

@seanyesmunt The issue I am struggling with, is that I (think) I need the <select> element to be a controlled component, and if so then it seems like it would be best to use internal state rather than the store, since no other component should care what option is selected unless this component decides to update the state of the store. The reason I am setting the state when the component mounts and when it receives props, is that if another component updates the store when a new channel is logged into, then I want this <select> element to re-render with that option selected. Any ideas on how I can approach this differently so that I can eliminate internal state while still accomplishing those goals?

@seanyesmunt The issue I am struggling with, is that I (think) I need the `<select>` element to be a controlled component, and if so then it seems like it would be best to use internal state rather than the store, since no other component should care what option is selected unless this component decides to update the state of the store. The reason I am setting the state when the component mounts and when it receives props, is that if another component updates the store when a new channel is logged into, then I want this `<select>` element to re-render with that option selected. Any ideas on how I can approach this differently so that I can eliminate internal state while still accomplishing those goals?

kauffj (Migrated from github.com) requested changes 2018-01-22 19:37:17 +01:00

kauffj (Migrated from github.com) left a comment

About 1/2 of a review

public/assets/js/generalFunctions.js

kauffj (Migrated from github.com) commented

2018-01-22 19:36:32 +01:00

This probably should not be a string based check.

public/assets/js/generalFunctions.js

kauffj (Migrated from github.com) commented

2018-01-22 19:36:51 +01:00

We should look at getting all of this out of the cookie entirely.

react/components/NavBarChannelDropdown.jsx

kauffj (Migrated from github.com) commented

2018-01-22 19:34:17 +01:00

Misnamed

react/utils/xhr.js

kauffj (Migrated from github.com) commented

2018-01-22 19:36:01 +01:00

All get and post requests should not assume a 401 means invalid username and password. Instead, have the server pass back the error message. Also, checks for 401 vs. 403 appear to be inconsistent.

React/Redux - publish component #323