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

PR de Correção (Não precisa ser mergeado) #10

Closed
wants to merge 23 commits into from
Closed

Conversation

soter19
Copy link

@soter19 soter19 commented Mar 16, 2020

andriusrl and others added 23 commits March 9, 2020 11:24
Layout com menu quase pronto
adicionado a funcao no componente de lista completo!
Projeto completo com alguns bugs
Comment on lines +45 to +49
constructor(props) {
super(props)
}

render() {
Copy link
Author

Choose a reason for hiding this comment

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

Poderia ser um componente funcional, vocês não fizeram uso do estado e de nenhum método de ciclo de vida (componentDidMount, componentWillUnmount).

Comment on lines +38 to +40
@media (max-width: 480px){
width: 100%;
}
Copy link
Author

Choose a reason for hiding this comment

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

Bom uso do media query!

Comment on lines +155 to +157
<ButonOrder onClick={() => this.filterAlphabetically()}>Ordernar Por Titulo</ButonOrder>
<ButonOrder onClick={() => this.filterByValue()}>Ordernar Por valor</ButonOrder>
<ButonOrder onClick={() => this.filterByDate()}>Ordernar Por Data</ButonOrder>
Copy link
Author

Choose a reason for hiding this comment

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

Quando temos esse tipo de situação (o método que lida com um evento não receber parâmetros), podemos fazer da seguinte forma:

<ButonOrder onClick={this.filterAlphabetically}>Ordenar Por Titulo</ButonOrder>
<ButonOrder onClick={this.filterByValue}>Ordenar Por valor</ButonOrder>
<ButonOrder onClick={this.filterByDate}>Ordenar Por Data</ButonOrder>

^ Essa é uma forma mais eficiente de fazer o que vocês fizeram acima, pois dessa forma vocês poupam a criação das arrow functions a cada iteração do render.

<Filtro>
<input placeholder="Filtrar por titulo..." value={this.state.title} onChange={this.inputTitle}></input>
<input placeholder="Filtrar por Descrição..." value={this.state.descricao} onChange={this.inputDescricao}></input>
<button onClick={() => this.filterListByTitleAndDescription(this.state.title, this.state.descricao)}>filtrar</button>
Copy link
Author

Choose a reason for hiding this comment

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

Atenção! Quando temos um método que precisa de informações que estão no estado do componente, não precisamos passá-las no render, exemplo:

// removi os parâmetros da função
filterListByTitleAndDescription = () => {
        let jobList = this.state.jobList.filter((job) => {
            return job.title.includes(this.state.title) // troquei aqui para pegar direto do estado
        })
        if (descricao !== "") {
            jobList = jobList.filter((job) => {
                return job.description.includes(this.state.descricao) // troquei aqui para pegar direto do estado
            })
        }
        this.setState({ filterList: jobList })
}

Dessa forma, limpamos o JSX no render que pode ficar assim:

<button onClick={this.filterListByTitleAndDescription}>filtrar</button>

<button onClick={() => this.filterListByTitleAndDescription(this.state.title, this.state.descricao)}>filtrar</button>
<input placeholder="valor minimo" value={this.state.valMin} onChange={this.inputValMin}></input>
<input placeholder="valor maximo" value={this.state.valMax} onChange={this.inputValMax}></input>
<button onClick={() => this.filterListByValMInAndValMax(this.state.valMin, this.state.valMax)}>filtrar</button>
Copy link
Author

Choose a reason for hiding this comment

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

Mesma coisa que comentei sobre o outro botão nesse componente.

render() {
const list = this.state.filterList

if (list !== undefined) {
Copy link
Author

Choose a reason for hiding this comment

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

Nos casos que temos checagens para garantir se uma lista pode ser mostrada, ao inverter a comparação o código fica mais simples, exemplo:

	if(list === undefined){
		return (<div>Lista vazia</div>)
	}

	return (
		// O componente de vocês aqui, fora de um if, mais fácil de ler.
	)

Comment on lines +99 to +132
<Input type="text"
onChange={this.saveTitle}
value={this.state.title}
></Input>
<StyledElemnts>Descrição</StyledElemnts>
<Input type="text"
onChange={this.saveDescription}
value={this.state.description}
></Input>
<StyledElemnts>Valor</StyledElemnts>
<Input type="number"
onChange={this.savePrice}
value={this.state.price}
></Input>
<StyledElemnts>Pagamento</StyledElemnts>
<NativeSelect
onChange={this.savePayment}
value={this.state.payment}
>
<option>Selecione a forma de pagamento</option>
<option>Cartão de crédito</option>
<option>Cartão de débito</option>
<option>Dinheiro</option>
<option>Cheque</option>
<option>Escambo</option>
</NativeSelect>
<StyledElemnts>Dia de agendamento</StyledElemnts>
<StyledDate type="date"
onChange={this.saveDate}
value={this.state.date}
></StyledDate>
<Button variant="outlined"
onClick={this.createJob}
>Criar contrato</Button>
Copy link
Author

Choose a reason for hiding this comment

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

Atenção para indentação e organização das props, dificulta a leitura.

switch (currentMenu) {
case 'initial':
return (
<Fragment>
Copy link
Author

Choose a reason for hiding this comment

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

Ótimo uso do Fragment!

Comment on lines +95 to +98
this.setState({
currentMenu: 'contract',
currentPage: 'contract'
})
Copy link
Author

Choose a reason for hiding this comment

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

Sei que é tentador, mas seria mais ideal se essa lógica estivesse extraída pra um método próprio.

Comment on lines +101 to +104
this.setState({
currentMenu: 'provider',
currentPage: 'listJobsProvider'
})
Copy link
Author

Choose a reason for hiding this comment

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

Sei que é tentador, mas seria mais ideal se essa lógica estivesse extraída pra um método próprio.


let jobList = this.state.jobList
jobList = this.state.jobList.filter((job) => {
if (min !== "") {
Copy link
Author

Choose a reason for hiding this comment

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

uma forma mais eficiente de fazer essa checagem é olhar o .length da string:

if(min.length > 0){
}

Comment on lines +134 to +138
jobList.sort(function (jobA, jobB) {
if (Date.parse(jobA.dueDate) < Date.parse(jobB.dueDate)) { return -1; }
if (Date.parse(jobA.dueDate) > Date.parse(jobB.dueDate)) { return 1; }
return 0;
})
Copy link
Author

Choose a reason for hiding this comment

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

Bom uso do sort, ficou claro!

}
{

this.state.taken ? <p><strong>Serviço já contratado</strong></p> : <p><strong>Serviço não contratado</strong></p>
Copy link
Author

Choose a reason for hiding this comment

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

Uma forma de simplificar essa lógica seria só trocar o texto, algo como:

const serviceStatusText = this.state.taken ? "Serviço já contratado" : "Serviço não contratado";

Aí o JSX ficaria assim:

	<p><strong>{serviceStatusText}</strong></p>

Repetindo menos código.

Comment on lines +7 to +52
const JobWrapper = styled.div`
display: flex;
justify-content: center;
align-items: center;
width: 60%;
margin: 10px auto;
min-height: 150px;
`
const InfoWrapper = styled.div`
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
padding: 5px;
width: 100%;
`

const DescicaoWrapper = styled.div`
padding: 5px;
text-align: center;
width: 200%;
`
const classes = {
card: {
minWidth: 275,
},
bullet: {
display: 'inline-block',
margin: '0 2px',
transform: 'scale(0.8)',
},
title: {
fontSize: 14,
},
pos: {
marginBottom: 12,
},
};

const Description = styled(Card)`
margin: 10px;
width: 100%;
`
const ItemContrato = styled(CardContent)`
display:flex;
`
Copy link
Author

Choose a reason for hiding this comment

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

Percebam que as primeiras 52 linhas desse arquivo foram só pra coisas secundárias, uma boa forma de se organizar seria extrair esses componentes para outro arquivo e só importá-los aí.

@soter19
Copy link
Author

soter19 commented Mar 17, 2020

Pontos de atenção - FutureNinjas

  • Boa adição dos itens na barra de navegação superiora!

  • Bom uso dos cards na lista de trabalhos, gostei da disposição das informações!

  • Sugestões de polimentos de UX:

    • Indicativo de carregamento no formulário de cadastro de trabalho.
    • Indicativo de sucesso ao cadastrar um trabalho, o usuário não consegue saber se acabou e se deu certo ou errado.
    • Substituir o texto "lista vazia" por "Carregando..." já proporcionaria uma experiência melhor pro usuário.
    • Uma vez colocado o valor mínimo de filtro não é possível resetar esse filtro, o usuário precisa digitar "0" no valor mínimo e mandar filtrar novamente.
    • Pode ser confuso para o usuário final ter 2 botões escritos "filtrar", acredito que transformar em um botão só deixaria mais claro.
  • Sugestões de polimento de UI:

    • o body do projeto está com a margem padrão de 8px, seria legal removê-la, vcs poderiam fazer isso usando o GlobalStyle do styled-components.
    • A div principal de vocês (onde vcs renderizam os trabalhos ou o formulário) poderia ter um min-height definido no CSS, para reduzir a mudança brusca de uma seção para outra, quando mudamos de seção atualmente, o footer sobe até o meio da página e desce novamente.
    • Sugeriria uma mudança no layout da barra de filtragens: colocar um espaçamento entre os campos de texto pois impediria toques acidentais quando usamos o touch de um celular por exemplo.

Código

  • Muitos console.log deixados para trás

  • Atenção para a indentação.

    • Dentro de JSX retornado em funções/métodos
    • CSS de componentes do styled-components

@soter19 soter19 closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants