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

Junção das função 'setYear' e 'setMonth' #57

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

gabrielsclimaco
Copy link
Contributor

Refatoração do componente SelectDate.js (#11)

Copy link
Contributor

@victorsenam victorsenam left a comment

Choose a reason for hiding this comment

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

Olá. Obrigado por contribuir!

Ele fez uma função mais genérica do que as antigas setMonth e setYear pra definir as datas. Achei que ficou bem melhor.

Testei aqui e parece tudo bem. Pra testar eu só fui no http://localhost:3000/#/data-nascimento e verifiquei que a parada ainda funciona e leva pra página certa quando clico em Continuar.
Fiz uma pergunta sobre o value lá, mas não vejo porque não aprovar e deixar aquilo pra um novo PR se a gente quiser tirar.

Seria legal descrever rapidamente como testou o PR da próxima vez que for contribuir pra ajudar na revisão.

<select
className="default-select"
id="month-of-birth"
value={this.state.monthOfBirth}
Copy link
Contributor

Choose a reason for hiding this comment

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

Essa linha aqui é necessária? Sei que já estava ai antes desse commit, mas consegue me dizer o motivo? haha

@ludimila
Copy link
Contributor

Obrigada @victorsenam e muuuuito obrigada @gabrielsclimaco (sdds :D)

@ludimila ludimila merged commit 23a02b0 into prefeiturasp:master Oct 23, 2018
@gabrielsclimaco
Copy link
Contributor Author

@victorsenam obrigado pelo feedback!

Sobre como eu testei: fiz da mesma forma que você descreveu, acessei a rota e verifiquei se o comportamento do componente se mantinha da mesma forma antes da refatoração. Nas próximas contribuições vou adicionar essa descrição no PR.

Com relação ao value, acredito que ele ainda seja necessário para que o select mostre o valor da opção selecionada.

@gabrielsclimaco
Copy link
Contributor Author

@ludimila sdds tb :)

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.

3 participants