-
Notifications
You must be signed in to change notification settings - Fork 401
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
Feature (API) - Adiciona query params para filtrar lista de conteúdos do usuário #1188
Feature (API) - Adiciona query params para filtrar lista de conteúdos do usuário #1188
Conversation
@ErickCReis is attempting to deploy a commit to the TabNews Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fc2bc38
to
079fccf
Compare
Sei que o foco da milestone não é esse, mas tive um tempinho para voltar a ver esse pr. Acabei decidindo remover os ajustes no front, acho que precisamos pensar um pouco mais em como deve ser o funcionamento, por conta disso preferi separar as coisas. Fiz mais alguns testes mas não consegui encontrar uma solução legal para resolver a paginação na tela de perfil. |
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.
Erick, isso é uma ótima adição ao projeto! Dei uma olhada no código e deixei algumas observações, a principal é sobre a mudança do nome showComments
. Me diga o que acha 👍
Sobre as sugestões para remover as variáveis não usadas: agora temos uma regra ativa do ESLint para isso, após atualizar sua branch com o main
você verá os erros.
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.
Realmente a funcionalidade desse PR é muito esperada 🎉. Infelizmente eu não dei conta de revisar no momento que ele foi aberto (nem na segunda versão), pois a demanda estava muito alta naquele período.
Mas vamos tentar levar ele para produção (ou abrir um novo, se for mais fácil). 🤝🚀
Front
Eu gostei muito do design! Talvez para adaptar ao que temos hoje, adicionaria a aba "Perfil", que seria a padrão se o usuário tiver cadastrado algo, e ao clicar nas outras abas navegaria para as respectivas páginas (root
ou child
), cada uma com sua paginação.
Provavelmente será melhor só mostrar a aba "Perfil" se o usuário cadastrou algo.
Endereços
Na /[username]
abrir a aba "Perfil" se o usuário tiver algo cadastrado, caso contrário, redirecionar para a "Conteúdos".
E para os "Conteúdos" e "Respostas", que tal:
Opção 1 - Mais próximo do atual
Talvez manter o padrão atual para os root
, onde apenas mudaria que para ver a primeira página raiz, obrigatoriamente precisaria ir para /[username]/pagina/1
, pois a /[username]
abriria só o perfil.
Para os comentários, talvez criar a /[username]/respostas/pagina/[page]
, ou algo melhor.
/[username]/pagina/[page]
/[username]/respostas/pagina/[page]
Opção 2 - Remover /pagina
Eu não vejo necessidade da /pagina
, e não lembro se houve alguma discussão sobre essa necessidade, então gostaria de saber a opinião sobre usar:
/[username]/conteudos/[page]
/[username]/respostas/[page]
Acredito que o plural seja suficiente para informar que /[username]/conteudos/3
não vai abrir o conteúdo 3, mas sim o bloco 3 de conteúdos.
Back
Para auxiliar nas ideias sobre o backend, recomendo darem uma olhada em alguns pontos do PR #1413 (especificamente o 5d06a21).
Pode ser que algo de lá seja aproveitado. Só não se assustem, pois aquele PR faz muito mais coisas, como otimizações e deixa o endpoint contents
flexível (dá até para buscar o conteúdo por ID). Inclusive alguns pontos dele já estão em produção. Acho que algumas coisas de lá podem ser aplicadas ao /api/v1/contents/[username]
.
Por exemplo, lá eu usei os parâmetros with_root
e with_children
. Então é possível pedir os dois tipos ou apenas um deles em uma mesma requisição.
Também recomendo dar uma olhada como eu fiz para poder fazer a consulta usando $not_null
, e não foi necessário adicionar o $null
.
…er user content list
c62190c
to
08c54b6
Compare
@Rafatcb e @aprendendofelipe muito obrigado pelos comentários. BackRealmente o FrontGostei bastante das sugestões do @aprendendofelipe, acho que faz sentido uma terceira aba para o perfil, mas ainda tenho algumas dúvidas:
Será que faz sentido separar front e back em prs distintos? O back me parece ser mais simples e já está mais avançado já com relação ao front acho que vai ser algo mais complexo.
Pra mim a opção 2 faz mais sentido |
Boa ideia! Acho que assim não quebra a interface atual da API. 👍
Acho que pode deixar só o username como cabeçalho de todas as abas/páginas. Daí elimina a necessidade de redirecionar se o usuário não tiver cadastrado o perfil, pois já vai aparecer no mínimo os saldos e o tempo de criação do usuário dentro da aba "Perfil". E não precisa listar os conteúdos nessa página. Acho que pode deixar apenas as listas separadas, ou criar uma nova aba em que sejam listados os dois tipos de publicações juntos, mas, a princípio, não vejo necessidade de manter uma versão igual a atual.
Show, separado é melhor mesmo! 👍 |
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.
Também acho melhor um PR separado para o front, isso facilitará a discussão também.
Deixei um comentário no PR só, que é uma dúvida minha.
.when('$required.with_children', { is: 'required', then: Joi.required(), otherwise: Joi.optional() }) | ||
.messages({ | ||
'any.required': `"with_children" é um campo obrigatório.`, | ||
'string.empty': `"with_children" não pode estar em branco.`, |
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.
Sei que esse não é o único lugar que tem a validação string.empty
em um campo boolean, mas quando ela é acionada? Nos testes que eu havia feito (antes das alterações do PR de hoje) não consegui simular um cenário em que ela fosse necessária.
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.
Acabei apenas copiando de outros schemas, lendo a documentação não encontrei nada a respeito, acredito que podemos remover.
Posso remover apenas desses dois schemas ou aproveito para remover dos outros tbm?
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.
Creio que poderia remover de todos campos booleanos, mas prefiro aguardar uma confirmação do @aprendendofelipe.
Não cheguei a testar se essa validação influencia em algo caso o parâmetro esteja no body
, testei apenas como parâmetro da query.
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.
Podemos remover sim a mensagem de string.empty
dos campos booleanos 👍
Mas acho que vamos fazer isso em outro PR, pois pra mim esse aqui está pronto 🚀
Atualização
Deixei apenas os ajustes de api nesse pr
Temos algumas issues sobre essa feature, então resolvi fazer uns testes aqui.
#807
#891
#1046
#1093