Mara y Fer / PR para Code Review :-) #17
Conversation
Starting react-app
* trying navbar * Nabvar with burger menu, doubt about css * changing burger-menu css Co-authored-by: Mara Mulato <[email protected]>
* trying navbar * Nabvar with burger menu, doubt about css * changing burger-menu css * new order pages, before pull commit * React-Router. Render conflict in progress Co-authored-by: Mara Mulato <[email protected]>
* trying navbar * Nabvar with burger menu, doubt about css * changing burger-menu css * new order pages, before pull commit * React-Router. Render conflict in progress * minor css changes on navbar and menu, react router ok * starting login, minor css changes and corrections Co-authored-by: Mara Mulato <[email protected]> Co-authored-by: Fernanda de la Peña ☆ <[email protected]>
…epending on how long they been created...
| const Forms = ({ history }) => { | ||
|
|
There was a problem hiding this comment.
| const Forms = ({ history }) => { | |
| import { useHistory } from 'react-router-dom'; | |
| const Forms = () => { | |
| const history = useHistory(); |
| } catch (err) { | ||
| alert(err); | ||
| } | ||
| }, [history]); |
There was a problem hiding this comment.
Por qué history es una dependencia de useCallback?, pueden elaborar un poco?
| } | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
parece que <> no sería de ayuda acá. Pienso que pueden removerlo.
There was a problem hiding this comment.
Gracias, me di cuenta que fuimos dejando varios <> innecesarios, los vamos a remover :-)
| return <Redirect to='/login' /> | ||
| } | ||
|
|
||
| const signOut = () => { auth.signOut() } |
There was a problem hiding this comment.
| const signOut = () => { auth.signOut() } | |
| const signOut = () => auth.signOut() |
| const signOut = () => { auth.signOut() } | ||
|
|
||
| return( | ||
| <> |
There was a problem hiding this comment.
parece que <> no sería de ayuda acá. Pienso que pueden removerlo.
| <img | ||
| className="logout" | ||
| onClick={signOut} | ||
| src={logoutIcon} | ||
| alt="logout icon"> | ||
| </img> |
There was a problem hiding this comment.
| <img | |
| className="logout" | |
| onClick={signOut} | |
| src={logoutIcon} | |
| alt="logout icon"> | |
| </img> | |
| <img | |
| className="logout" | |
| onClick={signOut} | |
| src={logoutIcon} | |
| alt="logout icon" /> | |
| return( breakfastData.map(elem => | ||
| <MenuElement data={elem} key={elem.id}/> | ||
| ) | ||
| ) |
There was a problem hiding this comment.
| return( breakfastData.map(elem => | |
| <MenuElement data={elem} key={elem.id}/> | |
| ) | |
| ) | |
| return breakfastData.map(elem => <MenuElement data={elem} key={elem.id} />) |
| function MenuElement (props) { | ||
| const {url, item, price, tag} = props.data | ||
| const addProduct = props.addProduct |
There was a problem hiding this comment.
| function MenuElement (props) { | |
| const {url, item, price, tag} = props.data | |
| const addProduct = props.addProduct | |
| function MenuElement ({ data: { url, item, price, tag }, addProduct}) { | |
| + | ||
| </button> | ||
| </div> | ||
| <h4>${price}.00</h4> |
There was a problem hiding this comment.
Creo que es una mejor idea usar Intl.NumberFormat para manejo de monedas
There was a problem hiding this comment.
Gracias, no sabía que eso existía, oops
| onChange={ e => { | ||
| const table = e.target.value; | ||
| setTable(table) | ||
| } | ||
| } |
There was a problem hiding this comment.
| onChange={ e => { | |
| const table = e.target.value; | |
| setTable(table) | |
| } | |
| } | |
| onChange={e => setTable(e.target.value)} |
| style={ | ||
| { backgroundColor: tab === 'breakfast' ? '#FFF' : '#DED2FF', | ||
| fontWeight: tab === 'breakfast' ? 'bold' : 'normal', | ||
| fontSize: tab === 'breakfast' ? '26px' : '25px' | ||
| } |
There was a problem hiding this comment.
Sería más legible realizarlo mediante classname
Ejemplo
Mis clases css
.toogle {
// ... some styles
background-color: #DED2FF;
font-weight: normal;
font-size: 25px;
}
.toggle--active {
background-color: #FFF;
font-weight: bold;
font-size: 26px;
}Y en mi component
<button className={`toggle ${tab === 'breakfast' ? 'toggle--active': ''}`} onClick={() => setTab('breakfast')}>There was a problem hiding this comment.
Lo intentamos hacer así, pero cuando dabamos click en otro lado de la pantalla (por ejemplo en los elementos para agregarlos al menú), el botón dejaba de estar activo y la clase cambiaba a la primera :(
| style={{ backgroundColor: tab === 'restOttd' ? '#FFF' : '#DED2FF', | ||
| fontWeight: tab === 'restOttd' ? 'bold' : 'normal', | ||
| fontSize: tab === 'restOttd' ? '26px' : '25px' | ||
| } |
| toast.warn('Asigna la mesa y productos a la orden 🍔🍟', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| transition: Slide | ||
| }); |
There was a problem hiding this comment.
Estas propiedades pueden ser declaradas en el top y reutilizadas para todos las notificaciones toast.
| toast.warn('Asigna la mesa y productos a la orden 🍔🍟', { | |
| className: "rounder-edges", | |
| position: "top-center", | |
| autoClose: 4000, | |
| hideProgressBar: true, | |
| transition: Slide | |
| }); | |
| toast.warn('Asigna la mesa y productos a la orden 🍔🍟', toastProps); |
There was a problem hiding this comment.
okis, el objeto con todas esas propiedades lo cambiamos a una carpeta que se llama utils, y lo mandamos a llamar en donde se necesita, gracias :- )
| if(order.items.length >0 && order.table <= 0){ | ||
| toast.warn('Por favor asigna la mesa', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| transition: Slide | ||
| }); | ||
| }else if(order.items.length === 0 && order.table >= 1){ | ||
| toast.warn('Agrega al menos un producto a la orden 🍔', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| transition: Slide | ||
| }); | ||
| }else if (order.items.length === 0 && order.table <= 0){ | ||
| toast.warn('Asigna la mesa y productos a la orden 🍔🍟', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| transition: Slide | ||
| }); | ||
| }else{ | ||
| await db | ||
| .collection('orders') | ||
| .add(order) | ||
| setOrder({...initialValues}) | ||
| toast.success('Pedido enviado a cocina! ✨', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| transition: Slide | ||
| }); | ||
| } |
There was a problem hiding this comment.
| if(order.items.length >0 && order.table <= 0){ | |
| toast.warn('Por favor asigna la mesa', { | |
| className: "rounder-edges", | |
| position: "top-center", | |
| autoClose: 4000, | |
| hideProgressBar: true, | |
| transition: Slide | |
| }); | |
| }else if(order.items.length === 0 && order.table >= 1){ | |
| toast.warn('Agrega al menos un producto a la orden 🍔', { | |
| className: "rounder-edges", | |
| position: "top-center", | |
| autoClose: 4000, | |
| hideProgressBar: true, | |
| transition: Slide | |
| }); | |
| }else if (order.items.length === 0 && order.table <= 0){ | |
| toast.warn('Asigna la mesa y productos a la orden 🍔🍟', { | |
| className: "rounder-edges", | |
| position: "top-center", | |
| autoClose: 4000, | |
| hideProgressBar: true, | |
| transition: Slide | |
| }); | |
| }else{ | |
| await db | |
| .collection('orders') | |
| .add(order) | |
| setOrder({...initialValues}) | |
| toast.success('Pedido enviado a cocina! ✨', { | |
| className: "rounder-edges", | |
| position: "top-center", | |
| autoClose: 4000, | |
| hideProgressBar: true, | |
| transition: Slide | |
| }); | |
| } | |
| if (order.items.length === 0) | |
| return toast.warn('Agrega al menos un producto a la orden 🍔',toastProps) | |
| if(order.table <= 0) | |
| return toast.warn('Por favor asigna la mesa', toastProps); | |
| await db.collection('orders').add(order) | |
| setOrder({...initialValues}) | |
| toast.success('Pedido enviado a cocina! ✨', toastProps); |
| const restOtdComponents = restOfTheDayData.map(elem => | ||
| <MenuElement addProduct={addProduct} data={elem} key={elem.id} /> | ||
| ) | ||
|
|
||
| // prints the breakfast components | ||
| const breakfastComponents = breakfastData.map(elem => | ||
| <MenuElement addProduct={addProduct} data={elem} key={elem.id}/> | ||
| ) | ||
|
|
||
| return( | ||
| <div className="menu-parent"> | ||
| <div className="menu-container" > | ||
| <ToggleMenu | ||
| className="toggle-btn" | ||
| setTab={setTab} | ||
| tab={tab} | ||
| /> | ||
|
|
||
| {tab === 'breakfast' ? <div className="items-container">{breakfastComponents}</div> : | ||
| <div className="items-container">{restOtdComponents}</div> | ||
| } | ||
| </div> |
There was a problem hiding this comment.
Acá esamos creando por adelantado ambas colecciones de components <MenuElement ...> tanto para restOtdComponents como para breakfastComponents, sola una colección debería ser creada dependiendo del tab seleccionado por lo cual esta sugerencia que les dejo es más conveniente y con mejor performance.
| const restOtdComponents = restOfTheDayData.map(elem => | |
| <MenuElement addProduct={addProduct} data={elem} key={elem.id} /> | |
| ) | |
| // prints the breakfast components | |
| const breakfastComponents = breakfastData.map(elem => | |
| <MenuElement addProduct={addProduct} data={elem} key={elem.id}/> | |
| ) | |
| return( | |
| <div className="menu-parent"> | |
| <div className="menu-container" > | |
| <ToggleMenu | |
| className="toggle-btn" | |
| setTab={setTab} | |
| tab={tab} | |
| /> | |
| {tab === 'breakfast' ? <div className="items-container">{breakfastComponents}</div> : | |
| <div className="items-container">{restOtdComponents}</div> | |
| } | |
| </div> | |
| return ( | |
| <div className="menu-parent"> | |
| <div className="menu-container"> | |
| <ToggleMenu className="toggle-btn" setTab={setTab} tab={tab} /> | |
| <div className="items-container"> | |
| {(tab === 'breakfast' ? breakfastData : restOfTheDayData).map(elem => | |
| <MenuElement addProduct={addProduct} data={elem} key={elem.id}/> | |
| )} | |
| </div> | |
| </div> |
There was a problem hiding this comment.
:0, gracias, no sabía que se estaba creando doble. Lo cambiamos y en próximos proyectos lo tendremos en cuenta :))
| addQuantity={addQuantity} | ||
| /> | ||
| ) )} | ||
| <p className="total">Total: ${addTotal}.00</p> |
There was a problem hiding this comment.
Es una mejor idea usar Intl.NumberFormat
| @@ -0,0 +1,168 @@ | |||
| import React, { useState, useEffect } from "react"; | |||
There was a problem hiding this comment.
El nombre del archivo creo que no relación con el nombre del componente Meal
src/components/MenuSelector/prueba.js ??
| onChange={(total) => { | ||
| let newProduct; | ||
| let add; | ||
| let totalPay; | ||
| if (product.find((p) => p.productId === item.uid)) { | ||
| newProduct = product.map((p) => { | ||
| if (p.productId !== item.uid) { | ||
| return p; | ||
| } | ||
| return { | ||
| ...p, | ||
| quant: total, | ||
| payment: total * item.price, | ||
| }; | ||
| }); | ||
| } else { | ||
| newProduct = [ | ||
| ...product, | ||
| { | ||
| productId: item.uid, | ||
| produItem: item.item, | ||
| unitaryPrice: item.price, | ||
| quant: total, | ||
| payment: total * item.price, | ||
| }, | ||
| ]; | ||
| } | ||
| setProduct(newProduct); | ||
| add = newProduct.reduce( | ||
| (sum, value) => sum + value.quant, | ||
| 0 | ||
| ); | ||
| setTotalQuantity(add); | ||
| { | ||
| } | ||
| totalPay = newProduct.reduce( | ||
| (sum, value) => sum + value.payment, | ||
| 0 | ||
| ); | ||
| setPayment(totalPay); | ||
| { | ||
| // console.log(totalPay); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Creo que sería una mejor idea separar la lógica de la app de la lógica de presentación (components), la idea sería como llevar esta lógica a una función fuera.
There was a problem hiding this comment.
Siempre he tenido problema con la modularización y eso. No sé si tengas algún recurso que me sirva para aprender a separar las funciones de los componentes, casi siempre lo pongo todo en el mismo archivo y se vuelve super confuso
| import { Link } from 'react-router-dom'; | ||
| import './BMenu.css'; | ||
|
|
||
| function BMenu(props) { |
There was a problem hiding this comment.
No es necesario declarar este param props porque no es usado en el componente.
| function BMenu(props) { | |
| function BMenu() { |
| toast.success('Pedidos actualizados', { | ||
| className: "rounder-edges", | ||
| position: "top-center", | ||
| autoClose: 4000, | ||
| hideProgressBar: true, | ||
| closeOnClick: true, | ||
| pauseOnHover: true, | ||
| draggable: true, | ||
| progress: undefined, | ||
| transition: Slide | ||
| }); |
There was a problem hiding this comment.
Colocar estas propiedades de los toast en el top sería mejor para evitar declararlos una y otra vez.
| // breakpoints for responsive CSS Masonry | ||
| const breakpointColumnsObj = { | ||
| default: 3, | ||
| 1100: 3, | ||
| 700: 2, | ||
| 500: 1 | ||
| }; |
There was a problem hiding this comment.
Colocar esta objeto con propiedades constantes en el top es una mejor idea para evitar recrear el objeto siempre que el componente haga un re-render.
| <div className="no-orders-message"> Aún no hay pedidos para entregar :-) </div> | ||
| </div> | ||
| ) | ||
| } else{ |
There was a problem hiding this comment.
No es necesario este bloque } else{ puesto que estamos haciendo un return dentro del bloque if más arriba.
Sería algo como
if (ready.length <=0) {
return (
<div className="no-orders-message-container">
<div className="no-orders-message"> Aún no hay pedidos para entregar :-) </div>
</div>
)
}
return(
<div className='container'>
<Masonry
breakpointCols={breakpointColumnsObj}
className="my-masonry-grid"
columnClassName="my-masonry-grid_column">
{readyOrders}
</Masonry>
</div>
)
| @@ -0,0 +1,38 @@ | |||
| import americanCoffee from '../img/american-coffee.webp' | |||
There was a problem hiding this comment.
Pienso que esta data debería venir del servidor, podriamos tener una colección products donde guardamos todos los productos disponibles, sugiero esto porque de esa forma el administrador del restaurante o negocio puede mantener (add/update/delete) productos de forma dinámica y conveniente.
Los contras de tener la data de forma estática sería que para realizar un cambio deberiamos hacerlo en el código y luego desplegar la app nuevamente. Esto no resulta conveniente para el negocio.
There was a problem hiding this comment.
síp, pensamos mas adelante añadir una sección en donde se puedan subir productos nuevos a la base de datos. Así que esto sería lo mas conveniente :D
| import 'firebase/firestore' | ||
| import 'firebase/auth' | ||
|
|
||
| var firebaseConfig = { |
There was a problem hiding this comment.
Mantengamos consistencia y usemos const o var. Creo que const aquí ajusta mejor.
There was a problem hiding this comment.
vale, esque así venía en la documentación de firebase y pues así se quedó.
| import 'firebase/auth' | ||
|
|
||
| var firebaseConfig = { | ||
| apiKey: "AIzaSyBLfsFNhBD1kNio0wyF0i1eesnf0MV-FE8", |
There was a problem hiding this comment.
No es una buena idea exponer keys en el código, creo que es una mejor idea usar variables de entorno (envars).
Ref: https://create-react-app.dev/docs/adding-custom-environment-variables/
There was a problem hiding this comment.
gracias! hay un pequeño detallito con esta parte y firebase, todas las keys que pongo en el .env las lee sin problema, excepto la de projectId, si no la pongo directamente en el file, la consola me dice que projectId tiene que ser un string y no jala.
There was a problem hiding this comment.
no sé que es lo que pasa en esa porque con todas las demas keys sirve.
| databaseURL: "https://burgerqueeeeeeen.firebaseio.com", | ||
| projectId: "burgerqueeeeeeen", | ||
| storageBucket: "burgerqueeeeeeen.appspot.com", | ||
| messagingSenderId: "650761219478", |
There was a problem hiding this comment.
Creo que no se esta haciendo uso de Firebase Notifications, por lo cual no es necesario este key messagingSenderId
| authDomain: "burgerqueeeeeeen.firebaseapp.com", | ||
| databaseURL: "https://burgerqueeeeeeen.firebaseio.com", | ||
| projectId: "burgerqueeeeeeen", | ||
| storageBucket: "burgerqueeeeeeen.appspot.com", |
There was a problem hiding this comment.
estamos haciendo uso de Firebase Storage?, en caso de que no creo que no sería necesario este key storageBucket
There was a problem hiding this comment.
La vamos a usar después para poder agregar los productos :)
| @@ -0,0 +1,13 @@ | |||
|
|
|||
There was a problem hiding this comment.
Parece que este componente no tiene más hijos que <Forms>, repensaría si realmente es necesario tener <Forms> como un componente, tal vez el contenido de este debería estar directamente en Login
Nos encantaría tener feedback sobre como modularizar un poco mas los componentes y en qué podemos mejorar, gracias :-)