-
Notifications
You must be signed in to change notification settings - Fork 0
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
#494 [FEAT]: Curadoria de respostas e aprimoramento do manuseio/apresentação de datas e localizações #507
Conversation
Co-authored-by: Izalorran Bonaldi <[email protected]>
#482 [FIX]: Dockerfile COPY dependência e package.json
…://github.com/VRI-UFPR/CienciaNaEscola into 494-feat-correcoes-de-gerenciamento-de-dados-de-data-e-localizacao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Excelente! Uma única ressalva quanto ao uso de um Popup aparentemente desnecessário em LocationInput (motivo da não aprovação); uma dúvida quanto a atualização dos valores default de TimeInput e DateInput; e, uma observação quanto ao seed dos protocolos já existentes.
@@ -549,60 +559,6 @@ function ApplicationPage(props) { | |||
} | |||
</div> | |||
); | |||
case 'DATEBOX': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No seed (main) ainda existem 9 DATEBOX, 8 TIMEBOX e 9 LOCATIONBOX o que causa o seguinte erro no front: input type not found. Eh interessante remover do seed também, e lembrar de solicitar a remoção de tais itens no banco em produção!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- De acordo quanto ao seed;
- Quanto ao servidor, os itens não podem ser diretamente removidos, uma vez que possuem respostas e dados neles contidos. Será necessário tratar a migração para o novo formato manualmente;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, ok!
const [currentPageIndex, setCurrentPageIndex] = useState(0); | ||
const [itemAnswerGroups, setItemAnswerGroups] = useState({}); | ||
const [answerDate, setAnswerDate] = useState(undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Se answerDate começa como undefined e só é atualizada com setAnswerDate que só é chamada quando se altera o valor da entrada, como a aplicação já aparece logo de cara com valores "atuais"?? Eh só uma dúvida mesmo, não peguei a ideia 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Excelente questão. A definição dos valores default para os componentes de data, horário e localização estão implementadas dentro dos próprios componentes, visando encapsulamento e coesão. Portanto, por mais que o componente pai, a página, guarde o estado da aplicação e de suas partes (além de fornecer as funções para atualizá-las), a tentativa foi para que tais chamadas ocorressem sempre a partir dos componentes responsáveis por tais partes (se houver);
- No caso específico da data, ver código;
- Um ponto a salientar é que, por melhor que tenha sido a intenção, esta não é a melhor forma de fazer isso. O ideal seria que os próprios componentes retivessem seu estado e o componente pai o solicitasse quando necessário (ao invés dos componentes filhos dispararem atualizações no componente pai, implicando em maior acoplamento). Acredito que isto possa ser feito adequadamente usando o hook useRef ou algo semelhante, conhecimento este que eu não possuía no momento da implementação original desta parte do sistema (e era mais feliz achando que estava certo, inesperada virtude da ignorância);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, entendido! Thanks 🙏 ! Acredito que essas alterações para useRef ou algo semelhante possam ser atribuídos a uma nova issue né?!
<Popup> | ||
<a className="color-dark-gray fw-bold fs-6" href="https://google.com"> | ||
Username - dd/mm/aaaa | ||
</a> | ||
</Popup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Não vejo necessidade desse Popup neste momento.. Faz sentido??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Falha minha! Era um popup de debug da página de respostas, acabei copiando e esquecendo de apagar.
Esta solicitação de pull inclui várias alterações na base de código, com foco principal na adição de novas dependências, simplificação da lógica do componente e integração do Leaflet para funcionalidades de mapa.
Atualizações de dependência:
package.json
: Adicionadas novas dependências, incluindoleaflet
,leaflet-defaulticon-compatibility
,react-app-rewired
ereact-leaflet
.Alterações no Dockerfile:
Dockerfile
: Adicionados arquivosconfig-overrides.js
e.env
à imagem do Docker.Simplificação da lógica do componente:
src/components/inputs/answers/DateInput.jsx
: Removidos props desnecessários e simplificada a funçãoupdateAnswer
. [1] [2]src/components/inputs/answers/TimeInput.jsx
: Foram removidos adereços desnecessários e simplificada a funçãoupdateAnswer
. [1] [2]Integração de Leaflet:
src/App.jsx
: Leaflet importado e estilos relacionados.src/components/inputs/answers/LocationInput.jsx
: Lógica de localização existente substituída pela integração do mapa Leaflet e componente simplificado. [1] [2] [3]Alterações adicionais:
src/pages/AnswerPage.jsx
: Adicionada referência de mapa e funcionalidade de aprovação de resposta. [1] [2] [3]