Remaining redux handling #3
Conversation
There was a problem hiding this comment.
Buenas! Esta muy bien!
Te hice algunos comentarios a corregir. Presta atención de utilizar ref en vez de findDOMNode.
Otra cosa que vi es que las Actions las estas creando dentro de cada page. Una de las cosas que tiene buenas redux (a mi entender) es te permite crear actions genericas que realizan una determinada tarea y que podes disparar de cualquier lado: crear timentry, crear proyecto, etc. Esto hace que si mañana tenes que crear un timeentry desde cualquier otro lado de la app, sea solo hacer el import y disparar la action.
Por lo tanto, no me parece bueno definir las actions dentro de cada page (de hecho, en todos los proyectos que hay por la vuelta nunca se hace). Si luego necesitas dispararla desde otra vista, estarias haciendo un import de una segunda vista, lo cual no tiene mucho sentido.
Espero haberme expresado bien 😂. Te recomiendo dejar las actions donde estan en el proyecto original, dentro de la carpeta donde esta la store.
Saludos y espero tus comentarios!
|
|
||
| class ProjectEntryList extends Component { | ||
| addProject() { | ||
| this.props.createProject({ | ||
| name: ReactDOM.findDOMNode(this.refs.projectNameInput).value |
There was a problem hiding this comment.
Usar ref. Intenta evitar usar findDOMNode
There was a problem hiding this comment.
Es un tema de react-bootstrap: react-bootstrap/react-bootstrap#1850
There was a problem hiding this comment.
ja, interesante. Habrá que dejarlo así entonces. Gracias por explicarmelo!
| @@ -14,30 +22,54 @@ class ProjectEntryList extends Component { | |||
| } | |||
|
|
|||
| render () { | |||
| const addProject = this.addProject.bind(this); | |||
There was a problem hiding this comment.
Como estamos usando stage-0 de Babel, podemos escribir esto de la siguiente forma:
::this.addProject
Por lo tanto, podemos evitar crear esta const y directamente en el JSX escribir:
onClick={::this.addProject}>
| return function(dispatch, getState) { | ||
| const state = getState(); | ||
| dispatch(doLogin(credentials)); | ||
| } |
There was a problem hiding this comment.
Que rol cumple atteptLogin?
Lo unico que veo que hace es funcionar de pasamano para disparar la action DO_LOGIN. Estas guardando el state en una variable que nunca se usa. Por lo tanto, porque no utilizar directamente la action doLogin?
Creo que simplemente login(credentials) quedaria mas simple y autoexplicativo 😄
There was a problem hiding this comment.
Mala mía, me quedo mal eso. Ahora lo actualizo. El attemptLogin lo que hace es despachar la acción doLogin (que hace la lógica de validación de login y asigna a loggedUser el valor del usuario si todo esta OK) y redirigir a /dashboard, si loggedUser es truth-y.
Lo separe así porque si lo hacia todo en una acción normal, nunca redirigía a /dashboard. Se me ocurrió que es porque, si bien las acciones son sincrónicas, es asincronico actualizar una prop que esta conectada con el state y por eso, loggedUser siempre daba null. Usando redux-thunk, no tuve el problema.
Respecto al nombre del método, me salio de adentro, creo que porque lo vi en alguna otra biblioteca.
|
|
||
| doLogin() { | ||
| this.props.attemptLogin({ | ||
| email: ReactDOM.findDOMNode(this.refs.emailInput).value |
| @@ -9,7 +9,7 @@ const routes = ( | |||
| <Route path='/' component={Layout}> | |||
| <IndexRedirect to='/login' /> | |||
| <Route path='/login' component={Login} /> | |||
| <Route path='/dashboard' component={Dashboard} /> | |||
| <Route path='/dashboard' name="dashboard" component={Dashboard} /> | |||
There was a problem hiding this comment.
Porque es necesario definir el name en este caso?
There was a problem hiding this comment.
Estuve viendo formas de redireccionar con react-router usando alias en lugar de poner la ruta entera dashboard en vez de /dashboard.
Me tome un tiempo y no lo hice marchar, así que decidí seguir de largo y esto quedo. Ya lo saco.
|
Diego, OK, me queda claro tu comentario de las Actions. Creo que me costo verlo en este proyecto porque es medio chico, pero ahora me doy cuenta de las ventajas que trae tener todas las actions centralizadas en un archivo. Hago el cambio. |
|
Fueron algunos fixes |
There was a problem hiding this comment.
Perfecto! Te hice un comentario chico pero esta impecable.
Sobre las actions, no necesariamente tienen que estar en el mismo archivo, pero si en un lugar común para todas las vistas. En este proyecto chico, en un archivo esta bien (al menos por el momento)
Saludos!
| @@ -7,7 +7,7 @@ import Select from 'react-select'; | |||
| import secToMin from 'sec-to-min'; | |||
| import moment from 'moment'; | |||
|
|
|||
| import { createTimeEntry } from '../../Actions/index'; | |||
| import { createTimeEntry } from '../../../../store/actions'; | |||
There was a problem hiding this comment.
cuando te quedan muchos ../ a veces lo mejor es escribir la ruta absoluto
src/store/actions
There was a problem hiding this comment.
Mmm, por alguna razon me escupe:
Module not found: Error: Can't resolve 'src/store/actions' in '/home/martin/workspace/js/workshop-react/src/pages/Dashboard/Components/TimeEntryForm' @ ./src/pages/Dashboard/Components/TimeEntryForm/TimeEntryForm.js 33:15-43
There was a problem hiding this comment.
Ya veo el problema, con el / se va literalmente al root del filesystem a buscar, por lo que funciona si pongo /home/martin/workspace/js/workshop-react/src/store/actions
Voy a buscar alguna otra alternativa (como un plugin o algo por el estilo), me gusta la idea de que el import absoluto vaya desde el root del proyecto.
|
Listo Diego.
EDIT: Acabo de agregar persistencia del |
@diegomura
Diego, te dejo lo que serian los últimos cambios de la stage 3. Lo que en particular me deja duda es como resolví el tema del usuario logueado (y como redirigir al dashboard una vez que se loguee el usuario) y el uso que le di a redux-thunk.
Muchas gracias!