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

SPA utilizando React #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RoxanaCS
Copy link

@RoxanaCS RoxanaCS commented Mar 7, 2018

No description provided.

@diegovelezg diegovelezg self-assigned this Mar 14, 2018
@diegovelezg diegovelezg added help wanted Extra attention is needed and removed help wanted Extra attention is needed labels Mar 14, 2018
Copy link

@enrique7mc enrique7mc left a comment

Choose a reason for hiding this comment

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

Hola, buen trabajo.

He dejado algunos comentarios que ayudarán a mejorar el código en cuanto a legibilidad y orden, si tienes alguna pregunta, no dudes en dejar tus comentarios.

<meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
<meta name="theme-color" content="#000000">
<link rel="manifest" href="%PUBLIC_URL%/manifest.json">
<!--<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.100.2/css/materialize.min.css">-->

Choose a reason for hiding this comment

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

Los estilos que no son utilizados pueden ser eliminados.

return (
<div className = "Footer">
<footer>
<p className = "footer-links">

Choose a reason for hiding this comment

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

<a href="url" los espacios no son necesarios entre el atributo y su valor. Aplica para todas las etiquetas que como div, p, a en este archivo.

<div className = "Header">
<div className = "Logo">
<img src = {logo} alt = "logo" className = "Logo-img" />
{/* <ul className = "menu">

Choose a reason for hiding this comment

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

El código comentado puede ser eliminado.

}

renderLoginButton() {
if (this.state.user) {

Choose a reason for hiding this comment

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

Tal vez una buena opción sea extraer el usuario desde this.state ya que se ocupa varias veces en este método:

const { user } = this.state;

</ul> */}
</div>
<div className = "ShoppingCart">
<img src = {cart} art = 'cart' className = 'ShopCart' alt = ""/>

Choose a reason for hiding this comment

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

El atributo alt es importante para la accesibilidad de los component, un valor simple como 'Shopping cart' podría ser útil.

render() {
return (
<div className="PicturesContainer">
{this.state.pictures[0]}

Choose a reason for hiding this comment

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

this.state.pictures es un arreglo de componentes, será posible mostrarlo directamente:

<div className="PicturesContainer">
    {this.state.pictures}
</div>

class MercadoLibreApp extends Component {
constructor() {
super();
this.state = { pictures: [],

Choose a reason for hiding this comment

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

Ajustar la indentación.

}

handleSubmit(event) {
// alert('A name was submitted: ' + this.state.value);

Choose a reason for hiding this comment

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

Comentario no necesario.

console.log(data);
let pictures = data.results.map((pic) => {
return (
<div className = "product" key = {pic.id}>

Choose a reason for hiding this comment

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

Este código para crear las imágenes se repite también en el componente Header, sería bueno poder extraerlo en un lugar común y usarlo desde los dos componentes. Eso creo que tomará un tiempo considerable, tal vez no tengas que cambiarlo ahora, pero tómalo en cuenta para futuros componentes.

};
render() {
return (
<div className = "container">

Choose a reason for hiding this comment

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

Arreglar la indentación de div y form

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants