All functionality is finally done. :) #19
Conversation
… firestore | toast messages
…| no orders order
It is, all the functionality, finally done
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
Como este archivo .css.map te es de ayuda ?
There was a problem hiding this comment.
Oh, en realidad no sabía que lo podía quitar... al trabajar con sass me di cuenta de que se generaba automáticamente y no le presté atención. Lo reviso.
|
|
||
| const Modal = ({isOpened, setOpened, setFlavor, onClickAccept}) => | ||
| isOpened ? | ||
| ReactDOM.createPortal( |
There was a problem hiding this comment.
Podrías elaborar como ReactDOM.createPortal funciona acá?
There was a problem hiding this comment.
Lo encontré como una solución para poder desplegar el modal en el contenedor principal.
There was a problem hiding this comment.
Seré algo crítico aquí. Este no es el tipo de respuesta que esperaría de un contribuidor al proyecto, menos del mantenedor y autor del PR.
Justo por ello te solicité que elaborarás un poco sobre CreatePortal para ayudarme a entender como funciona y como es beneficioso en tu código versus la otra forma convencional de hacerlo.
Creeme en muchas revisiones nos vemos forzados a elaborar acompañando de casos de uso, benchmarks, ejemplos de uso que dan soporte a nuestro PR, referencias, etc, y esto es más usual cuando insertamos nuevos conceptos, técnicas, apis externas las cuales podrían ser no familiares a otros contribuidores del codebase.
There was a problem hiding this comment.
De acuerdo con Raúl, tampoco tengo claro como se usa createPortal y sería bueno tener más información al respecto.
There was a problem hiding this comment.
Oh, lo siento. Usé un create portal porque entiendo que crea un portal para poder pasar entre jerarquías de componentes. La idea del portal sería evitar usar un z-index que es lo que usualmente se usa para los modals en vanilla js.
| if(orderSelected.preparando == true){ | ||
|
|
||
| let a; | ||
| let b; | ||
| let c; | ||
|
|
||
| setTimeout(() => { | ||
|
|
||
| if(sec == 59){ | ||
| setSec(0); | ||
| if(min ==59){ | ||
| setMin(0) | ||
| setHour(hour+1) | ||
| }else{ | ||
| setMin(min+1) | ||
| } | ||
|
|
||
| }else{ | ||
| setSec(sec+1) | ||
| } | ||
| }, 1000); | ||
|
|
||
| if(hour<10){ | ||
| a = '0' | ||
| }else{ | ||
| a = '' | ||
| } | ||
| if(min<10){ | ||
| b = '0' | ||
| }else{ | ||
| b = '' | ||
| } | ||
| if(sec<10){ | ||
| c = '0' | ||
| }else{ | ||
| c = '' | ||
| } |
There was a problem hiding this comment.
Es muy díficil tratar de seguir la lógica, creo que sería buena idea separar la lógica del timer de la lógica de presentación (componente) . Se me ocurre colocar esta función en un utils folder.
Creo realmente puedes hacerlo mejor, así que adelante una siguiente iteración para buscar hacerlo más legible.
There was a problem hiding this comment.
En esta parte tengo algunas dudas. Me fue un poco complicado resolver como renderear el time con el formato que deseaba tipo reloj digital. Voy a revisar de qué manera separar esa parte. Por otro lado, me doy cuenta que en la mayoría de mis funciones, sobre todo aquellas que me llevo a otras carpetas, me tengo que estar llevando muchos parámetros, ¿eso es una mala práctica?
Gracias. :)
There was a problem hiding this comment.
Si tus funciones necesitan de muchos parámetros, puede ser síntoma de que esta haciendo muchas cosas, tal vez sea neceasario repensar si puede ser divido en 2 o más funciones.
Otra de las cosas que pueden ayudarte es en lugar de usar párametros por posición es definir un objecto como único param.
Ejemplo:
// No muy buena idea
function doSomething(paramA, paramB, paramC, paramD) { ...
// Esto puede ser mejor donde objArgs recibirá como argumento un objecto { paramA, paramB, paramC, paramD }
function doSomething(objArgs) { ....Acá te dejo una refencia muy buena sobre esto https://blog.bitsrc.io/javascript-why-named-arguments-are-better-than-positional-arguments-9b15ab3155ef
| import firebase from 'firebase'; | ||
|
|
||
| const config = { | ||
| apiKey: "AIzaSyAvtcRBtUmWHHghCTbrm8ZCiWQmG8Yp5Sw", |
There was a problem hiding this comment.
No es una buena idea por seguridad exponer la apiKey de tu proyecto de Firebase en tu codebase, el lugar más idóneo es una variable de entorno.
Ref: https://create-react-app.dev/docs/adding-custom-environment-variables/
| databaseURL: "https://burger-queen-e24cd.firebaseio.com", | ||
| projectId: "burger-queen-e24cd", | ||
| storageBucket: "burger-queen-e24cd.appspot.com", | ||
| messagingSenderId: "461310075289", |
There was a problem hiding this comment.
Si no usamos Firebase Cloud Messaging esta línea no sería necesaria.
| authDomain: "burger-queen-e24cd.firebaseapp.com", | ||
| databaseURL: "https://burger-queen-e24cd.firebaseio.com", | ||
| projectId: "burger-queen-e24cd", | ||
| storageBucket: "burger-queen-e24cd.appspot.com", |
There was a problem hiding this comment.
Si no usamos Firebase Storage esta línea no sería necesaria
raulingg
left a comment
raulingg
left a comment
There was a problem hiding this comment.
Gran trabajo Fabi 💯 !!
He dejado varios comentarios a lo largo de todo tu codebase, espero que los encuentres de mucha ayuda para seguir mejorando.
corrections code review | first react and firebase deployment
| } | ||
|
|
||
| export default App; | ||
|
|
There was a problem hiding this comment.
Cuida tu indentación y los espacios que dejas para que tu archivo se vea más ordenado.
| $sColor: #161616; | ||
|
|
||
| $radius: 15px; | ||
|
|
There was a problem hiding this comment.
Es preferible tener un archivo en donde estén tus variables y luego solo importas el archivo a tus estilos.
There was a problem hiding this comment.
Gracias. De hecho justo creo que no exploté al 100 las ventajas de sass.
| /* | ||
| div{ | ||
| border: 1px solid red; | ||
| }*/ |
There was a problem hiding this comment.
Elimina código que no vas a utilizar.
| width: 100vw; | ||
| height: 100vh; | ||
| background-image: url(../images/background.png); | ||
| /*margin-top: 10%;*/ |
|
|
||
|
|
||
| export default function Button({className,value,onClick}) { | ||
|
|
| import 'firebase/firestore' | ||
|
|
||
| export function cancelOrderFromKitchen(id){ | ||
| console.log('Cancelando', id) |
There was a problem hiding this comment.
Evita subir tus concole.log esto es más para nosotros cuando estamos desarrollando.
| <td></td> | ||
| <td className="table-total"><span>Total: </span></td> | ||
| <td className="table-total-cant"><span>$ {orderSelected.total}</span></td> | ||
| </tr> |
There was a problem hiding this comment.
Estas líneas muy pegadas a la izquierda son síntoma de un mal formato en tus archivos.
| export const calcTotal = (products) => products.reduce((sum, curr) => sum + curr.price, 0); | ||
|
|
||
| export function addProduct(cardItem, products, setProducts){ | ||
| // el item ya esta |
| number: 1, | ||
| price: cardItem.price, | ||
| uPrice: cardItem.price | ||
| }]) |
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Aquí hay algunos espacios innecesarios.
|
|
||
| return ( | ||
| <Switch> | ||
| {islogin===true? |
There was a problem hiding this comment.
Aquí puedes simplificar poniendo solo isLogin, poner isLogin === true es redudante.
| </p> | ||
| <img src={image} alt=""/> | ||
| <p> | ||
| ${price} |
There was a problem hiding this comment.
Me parece que el signo de $ está de más.
| statusKitchen={statusKitchen} | ||
| statusOrders={statusOrders} | ||
| ordersAlert={ordersAlert} | ||
| /> |
There was a problem hiding this comment.
Dale a las props un tab para mandarlos a la derecha.
| setMin={setMin} | ||
| hour={hour} | ||
| setHour={setHour} | ||
| /> |
Readme images
gif and logo in readme images
3 participants
No description provided.