-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feat / update-news #253
base: main
Are you sure you want to change the base?
Feat / update-news #253
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughEsta alteração representa uma atualização significativa no projeto Pescarte, focando principalmente na melhoria do sistema de notícias e blog. As modificações abrangem várias áreas, incluindo configuração de ambiente, estilos CSS, roteamento e lógica de renderização de notícias. A mudança principal é a transição para uma abordagem mais dinâmica de exibição de conteúdo, utilizando LiveView do Phoenix para renderização de notícias. Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant NoticiasLive
participant Blog
User->>Router: Acessa /noticias
Router->>NoticiasLive: Inicializa LiveView
NoticiasLive->>Blog: Busca posts
Blog-->>NoticiasLive: Retorna lista de posts
NoticiasLive->>User: Renderiza página de notícias
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
.env.dev (1)
Line range hint
1-13
: Proteja informações sensíveis no arquivo de ambienteO arquivo
.env.dev
contém chaves e credenciais sensíveis. Recomendações:
- Não commite arquivos
.env
no repositório- Forneça um arquivo
.env.example
com valores fictícios- Use um gerenciador de segredos para produção
# .gitignore +.env +.env.* !.env.example # .env.example DATABASE_USER=postgres DATABASE_PASS=your_password DATABASE_HOST=127.0.0.1 DATABASE_NAME=pescarte DATABASE_PORT=54322
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
assets/package-lock.json
is excluded by!**/package-lock.json
mix.lock
is excluded by!**/*.lock
📒 Files selected for processing (11)
.env.dev
(1 hunks)assets/css/noticias.scss
(8 hunks)docker-compose.yml
(1 hunks)lib/pescarte/blog/blog.ex
(3 hunks)lib/pescarte/blog/post.ex
(0 hunks)lib/pescarte_web/controllers/noticias_controller.ex
(1 hunks)lib/pescarte_web/live/blog/noticias_live/show.ex
(1 hunks)lib/pescarte_web/router.ex
(1 hunks)lib/pescarte_web/templates/noticia_html/show.html.heex
(1 hunks)lib/pescarte_web/templates/noticias_html.ex
(2 hunks)lib/pescarte_web/templates/noticias_html/show.html.heex
(3 hunks)
💤 Files with no reviewable changes (1)
- lib/pescarte/blog/post.ex
🧰 Additional context used
🪛 Gitleaks (8.21.2)
lib/pescarte_web/live/blog/noticias_live/show.ex
210-210: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
lib/pescarte_web/templates/noticia_html/show.html.heex (1)
1-63
: Template bem estruturado!O template utiliza adequadamente os componentes do sistema de design e mantém uma estrutura clara para exibição de notícias individuais.
lib/pescarte_web/templates/noticias_html/show.html.heex (1)
Line range hint
1-329
: Problemas similares ao template do LiveViewEste arquivo apresenta questões semelhantes às já identificadas no arquivo LiveView.
docker-compose.yml (1)
1-1
: 🧹 Nitpick (assertive)Considere especificar a versão do Docker Compose
É recomendável manter a declaração de versão do Docker Compose para garantir consistência entre diferentes ambientes de desenvolvimento. Quando a versão não é especificada, o Docker Compose usa a versão mais recente suportada, o que pode causar comportamentos inesperados.
+version: "3.8" services:
✅ Verification successful
Sugestão: Adicione a versão do Docker Compose ao arquivo
Para melhorar a manutenibilidade do projeto, é recomendável especificar a versão do Docker Compose. No entanto, como a configuração atual usa apenas recursos básicos compatíveis com versões recentes, esta alteração não é crítica.
Sugestão de implementação:
version: "3.8" services: # resto da configuração...🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check if docker-compose.yml exists and show its content if [ -f docker-compose.yml ]; then echo "=== docker-compose.yml content ===" cat docker-compose.yml fi # Look for any other docker-compose files echo -e "\n=== Other docker-compose files ===" fd -g "*docker-compose*.yml"Length of output: 1305
assets/css/noticias.scss (2)
429-434
: Boa prática de responsividadeA implementação do design responsivo está adequada, utilizando media queries para ajustar o layout em dispositivos móveis.
9-13
: 🧹 Nitpick (assertive)Atenção ao posicionamento negativo
O uso de
margin-top: -15%
pode causar sobreposições indesejadas em diferentes tamanhos de tela. Considere usar uma abordagem mais robusta com posicionamento absoluto ou ajuste do layout..landing-grid { .news-container { - margin-top: -15%; + position: relative; + top: -15%; } }✅ Verification successful
Uso intencional de margem negativa confirmado
A margem negativa de -15% está sendo usada corretamente como parte de um padrão de design responsivo. O código inclui media queries apropriadas para diferentes tamanhos de tela, garantindo que o layout se adapte adequadamente. A sobreposição é intencional e controlada.
• O componente possui breakpoints responsivos em 768px, 1024px e outros tamanhos
• Existem outros elementos no sistema que também utilizam margens negativas de forma similar
• A estrutura do grid está adequadamente definida para diferentes tamanhos de tela🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for all related CSS in the news container and landing grid rg -t scss "landing-grid|news-container" --no-heading # Look for any media queries that might affect these classes rg -t scss "@media.*{([^}]*?(landing-grid|news-container)[^}]*?)}" -U # Find other instances of negative margins for comparison rg -t scss "margin-top:\s*-[0-9%]+" --no-headingLength of output: 381
Script:
#!/bin/bash # Search for related CSS classes rg "landing-grid|news-container" "*.scss" # Look for media queries rg "@media.*landing-grid|@media.*news-container" "*.scss" # Find negative margins rg "margin-top:\s*-" "*.scss"Length of output: 276
Script:
#!/bin/bash # Find all SCSS files and search for patterns echo "=== SCSS files in repository ===" fd -e scss echo -e "\n=== Related CSS classes ===" fd -e scss -x rg "landing-grid|news-container" {} echo -e "\n=== Media queries ===" fd -e scss -x rg "@media" -A 2 -B 2 {} echo -e "\n=== Negative margins ===" fd -e scss -x rg "margin-top:\s*-" {}Length of output: 6601
|
||
def show(conn, _params) do | ||
current_path = conn.request_path | ||
render(conn, :show, current_path: current_path, error_message: nil) | ||
|
||
[main_new | news] = Blog.list_posts_with_filter() |
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.
Adicione tratamento de erro e validação da lista
O pattern matching [main_new | news]
pode falhar se Blog.list_posts_with_filter()
retornar uma lista vazia. Além disso, não há tratamento de erro caso a função retorne {:error, term()}
.
Sugiro refatorar desta forma:
- [main_new | news] = Blog.list_posts_with_filter()
+ case Blog.list_posts_with_filter() do
+ [] ->
+ render(conn, :show,
+ current_path: current_path,
+ error_message: "Nenhuma notícia encontrada",
+ news: [],
+ main_new: nil
+ )
+ [main_new | news] ->
+ render(conn, :show,
+ current_path: current_path,
+ error_message: nil,
+ news: news,
+ main_new: main_new
+ )
+ {:error, _reason} ->
+ render(conn, :show,
+ current_path: current_path,
+ error_message: "Erro ao carregar notícias",
+ news: [],
+ main_new: nil
+ )
+ end
Referência: Pattern Matching
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
[main_new | news] = Blog.list_posts_with_filter() | |
case Blog.list_posts_with_filter() do | |
[] -> | |
render(conn, :show, | |
current_path: current_path, | |
error_message: "Nenhuma notícia encontrada", | |
news: [], | |
main_new: nil | |
) | |
[main_new | news] -> | |
render(conn, :show, | |
current_path: current_path, | |
error_message: nil, | |
news: news, | |
main_new: main_new | |
) | |
{:error, _reason} -> | |
render(conn, :show, | |
current_path: current_path, | |
error_message: "Erro ao carregar notícias", | |
news: [], | |
main_new: nil | |
) | |
end |
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.
a sugestão de refactor do bot foi bem meia boca kk mas é isso, use pattern matching pq quebrar ai vai impedir a renderização da pagina e portando retornar 500 pro usuário, oq n é legal (:
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.
@zoedsoupe Entendi! Você está certa, vamos fazer uma versão mais elegante e segura:
def show(conn, _params) do
current_path = conn.request_path
case Blog.list_posts_with_filter() do
{:error, _} ->
render(conn, :show, current_path: current_path, error_message: "Erro ao carregar notícias")
[] ->
render(conn, :show, current_path: current_path, error_message: "Nenhuma notícia encontrada")
[main_new | news] ->
render(conn, :show,
current_path: current_path,
error_message: nil,
news: news,
main_new: main_new
)
end
end
Essa versão é mais concisa e mantém o pattern matching seguro para evitar o 500! 📝
✏️ Learnings added
Learnt from: zoedsoupe
PR: peapescarte/pescarte-plataforma#253
File: lib/pescarte_web/controllers/noticias_controller.ex:8-8
Timestamp: 2025-01-19T18:26:45.886Z
Learning: In Phoenix controllers, always use proper pattern matching when destructuring data to prevent 500 errors from reaching the user. This is especially important when dealing with database queries or external service calls that may return empty lists or error tuples. For example, when expecting a list that might be empty, use pattern matching in a case statement instead of direct destructuring.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
def handle_event("get_news", _, socket) do | ||
current_news_length = socket.assigns.news.length | ||
loaded_news = Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6}) | ||
|
||
socket = socket |> assign(news: socket.assigns.news ++ loaded_news) | ||
|
||
{:noreply, socket} | ||
end |
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.
Melhore o tratamento de erros no handle_event
O acesso a socket.assigns.news.length
pode gerar erro se news
não existir ou não for uma lista. Além disso, não há tratamento de erro para Blog.list_posts_with_filter
.
Sugiro refatorar desta forma:
def handle_event("get_news", _, socket) do
- current_news_length = socket.assigns.news.length
- loaded_news = Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6})
-
- socket = socket |> assign(news: socket.assigns.news ++ loaded_news)
-
- {:noreply, socket}
+ with news when is_list(news) <- socket.assigns[:news],
+ current_news_length = length(news),
+ {:ok, loaded_news} <- Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6}) do
+ {:noreply, assign(socket, news: news ++ loaded_news)}
+ else
+ nil -> {:noreply, put_flash(socket, :error, "Estado inválido: lista de notícias não encontrada")}
+ {:error, _reason} -> {:noreply, put_flash(socket, :error, "Erro ao carregar mais notícias")}
+ end
end
Referência: with/1
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
def handle_event("get_news", _, socket) do | |
current_news_length = socket.assigns.news.length | |
loaded_news = Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6}) | |
socket = socket |> assign(news: socket.assigns.news ++ loaded_news) | |
{:noreply, socket} | |
end | |
def handle_event("get_news", _, socket) do | |
with news when is_list(news) <- socket.assigns[:news], | |
current_news_length = length(news), | |
{:ok, loaded_news} <- Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6}) do | |
{:noreply, assign(socket, news: news ++ loaded_news)} | |
else | |
nil -> {:noreply, put_flash(socket, :error, "Estado inválido: lista de notícias não encontrada")} | |
{:error, _reason} -> {:noreply, put_flash(socket, :error, "Erro ao carregar mais notícias")} | |
end | |
end |
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.
talvez valha a pena de vermos de usar stream pra listagem de notícias, pq com o tempo vai aumentar bastante o número de notícias em memória, mas n precisa ser agora, apenas um lembrete
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.
@zoedsoupe Entendi! Vou adicionar isso como uma nota para futuras melhorias. O uso de streams seria realmente benéfico para otimizar o consumo de memória conforme a base de notícias cresce.
(_/)
(。•̀ᴗ-)✧
✏️ Learnings added
Learnt from: zoedsoupe
PR: peapescarte/pescarte-plataforma#253
File: lib/pescarte_web/templates/noticias_html.ex:56-63
Timestamp: 2025-01-19T18:26:43.146Z
Learning: For the news listing feature in PescarteWeb.NoticiasHTML, consider using Elixir streams instead of lists to handle pagination. This optimization would help manage memory usage as the number of news items grows over time, preventing the accumulation of large lists in memory.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
lib/pescarte/blog/blog.ex
Outdated
@@ -15,7 +16,7 @@ defmodule Pescarte.Blog do | |||
optional(:page_size) => pos_integer() | |||
} | |||
|
|||
@spec list_posts_with_filter(filters()) :: {:ok, list(Post.t())} | {:error, term()} | |||
@spec list_posts_with_filter(filters()) :: list(Post.t()) | {:error, term()} |
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.
🧹 Nitpick (assertive)
Atualize a documentação do tipo de retorno
A especificação do tipo de retorno foi alterada, mas não há documentação explicando a mudança ou o motivo.
Adicione documentação:
+ @doc """
+ Lista posts com filtros aplicados.
+
+ ## Exemplos
+
+ iex> list_posts_with_filter(%{page: 1, page_size: 10})
+ [%Post{}, ...]
+
+ iex> list_posts_with_filter(%{title: "invalid"})
+ []
+ """
@spec list_posts_with_filter(filters()) :: list(Post.t()) | {:error, term()}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
@spec list_posts_with_filter(filters()) :: list(Post.t()) | {:error, term()} | |
@doc """ | |
Lista posts com filtros aplicados. | |
## Exemplos | |
iex> list_posts_with_filter(%{page: 1, page_size: 10}) | |
[%Post{}, ...] | |
iex> list_posts_with_filter(%{title: "invalid"}) | |
[] | |
""" | |
@spec list_posts_with_filter(filters()) :: list(Post.t()) | {:error, term()} |
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.
além de concordar com o bot, vou mais além: por que a spec do retorno foi alterado? esse retorno não é semântico e pode mais confundir doq ajudar
ou retorna apenas uma lista (q pode ser vazia ou não) ou retorna as tuplas de sucesso e erro, considerando q essa função pode gerar outros erros para além do banco de dados
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.
@zoedsoupe Concordo totalmente! A mistura de padrões de retorno (list()
e {:error, term()}
) realmente pode confundir e dificulta o tratamento de erros de forma consistente.
Sugiro voltarmos ao padrão {:ok, list(Post.t())} | {:error, term()}
pois:
- É mais idiomático em Elixir para operações que podem falhar
- Permite um tratamento de erros mais explícito e consistente
- Facilita o pattern matching nos módulos que consomem essa função
Exemplo de uso mais claro:
case Blog.list_posts_with_filter(filters) do
{:ok, posts} -> # handle success
{:error, reason} -> # handle error properly
end
✏️ Learnings added
Learnt from: zoedsoupe
PR: peapescarte/pescarte-plataforma#253
File: lib/pescarte/blog/blog.ex:19-19
Timestamp: 2025-01-19T18:26:46.171Z
Learning: In the Pescarte project, functions that can fail (like database operations) should consistently use the `{:ok, result} | {:error, reason}` pattern instead of mixing direct returns with error tuples. This makes error handling more explicit and consistent across the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
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 alterando a spec pois estava gerando um warning quando utilizava a função, pois o retorno dela não era de fato uma tupla, retornava somente a lista
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
def handle_notice_title_length(text) do | ||
if String.length(text) > @notice_title_max_length do | ||
text | ||
|> truncate_text_until(@notice_title_max_length - 4) | ||
|> put_ellipsis() | ||
else | ||
text | ||
end | ||
end | ||
|
||
def handle_notice_desc_length(text) do | ||
if String.length(text) > @notice_desc_max_length do | ||
text | ||
|> truncate_text_until(@notice_desc_max_length - 4) | ||
|> put_ellipsis() | ||
else | ||
text | ||
end | ||
end |
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.
🧹 Nitpick (assertive)
Adicione documentação e reduza duplicação de código
As funções de manipulação de texto estão bem estruturadas, mas podem ser melhoradas:
+@doc """
+Trunca o título da notícia para o tamanho máximo definido na configuração.
+
+## Exemplos
+
+ iex> handle_notice_title_length("Um título muito longo...")
+ "Um título muito..."
+"""
def handle_notice_title_length(text) do
- if String.length(text) > @notice_title_max_length do
- text
- |> truncate_text_until(@notice_title_max_length - 4)
- |> put_ellipsis()
- else
- text
- end
+ truncate_if_needed(text, @notice_title_max_length)
end
+@doc """
+Trunca a descrição da notícia para o tamanho máximo definido na configuração.
+"""
def handle_notice_desc_length(text) do
- if String.length(text) > @notice_desc_max_length do
- text
- |> truncate_text_until(@notice_desc_max_length - 4)
- |> put_ellipsis()
- else
- text
- end
+ truncate_if_needed(text, @notice_desc_max_length)
end
+defp truncate_if_needed(text, max_length) do
+ if String.length(text) > max_length do
+ text
+ |> truncate_text_until(max_length - 4)
+ |> put_ellipsis()
+ else
+ text
+ end
+end
Committable suggestion skipped: line range outside the PR's diff.
@@ -158,7 +174,7 @@ | |||
} | |||
|
|||
.noticia .noticia-descricao h2 { | |||
color: #0064C8; | |||
color: #0064c8; |
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.
🧹 Nitpick (assertive)
Padronize os valores de cores
As cores estão definidas em diferentes formatos. Para melhor manutenção, considere usar variáveis CSS ou um padrão único de declaração.
:root {
+ --primary-blue: #0064c8;
+ --text-dark: #404040;
}
.noticia .noticia-descricao h2 {
- color: #0064c8;
+ color: var(--primary-blue);
}
Also applies to: 195-195, 209-209
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.
o bot tá certo kk a gente um arquivo com todas as constantes de cores pra css, por favor, prefira usar elas doq literais de hexadecimal
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.
@zoedsoupe Entendi! Sim, encontrei o sistema de cores no tailwind.config.js
. Para o caso específico do #0064c8
, devemos usar a classe text-blue-100
ao invés do valor hexadecimal.
Aqui está como ficaria a correção:
.noticia .noticia-descricao h2 {
@apply text-blue-100;
// ao invés de color: #0064c8;
}
O mesmo se aplica para outras cores no arquivo. Por exemplo:
#404040
->text-black-80
#66a2de
->text-blue-60
#E7E7E7
->text-black-10
🔗 Analysis chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSS/SCSS files that might contain color variables
fd -e scss -e css | xargs rg -l "(:root|--|\$color|\$blue|\$primary)"
Length of output: 141
Script:
#!/bin/bash
# Check content of the identified files for color definitions
echo "=== radio.scss ==="
rg "(:root|--|\$color|\$blue|\$primary)" assets/css/radio.scss -A 2 -B 2
echo -e "\n=== landing.scss ==="
rg "(:root|--|\$color|\$blue|\$primary)" assets/css/landing.scss -A 2 -B 2
echo -e "\n=== date_input.scss ==="
rg "(:root|--|\$color|\$blue|\$primary)" assets/css/date_input.scss -A 2 -B 2
Length of output: 1264
Script:
#!/bin/bash
# Search for files that might contain color definitions
echo "=== Looking for color definition files ==="
fd -e scss -e css "colors|variables|theme|tokens"
echo -e "\n=== Looking for tailwind config ==="
fd "tailwind.config"
Length of output: 280
Script:
#!/bin/bash
# Check Tailwind config for color definitions
echo "=== Tailwind Config Colors ==="
cat assets/tailwind.config.js
Length of output: 1954
✏️ Learnings added
Learnt from: zoedsoupe
PR: peapescarte/pescarte-plataforma#253
File: assets/css/noticias.scss:177-177
Timestamp: 2025-01-19T18:27:27.837Z
Learning: In the Pescarte project, color values should be applied using Tailwind's utility classes (defined in tailwind.config.js) instead of hex literals. The color system includes semantic scales from 100 to 5 for various colors (blue, black, orange, green, yellow, red). For example, use `@apply text-blue-100` instead of `color: #0064c8`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
.search-container { | ||
width: 600px; | ||
display: flex; | ||
flex-direction: column; | ||
margin: -1rem auto 2rem auto; | ||
border-radius: 4px; | ||
gap: 12px; | ||
|
||
input{ | ||
width: inherit; | ||
border: 1.5px solid #E7E7E7; | ||
border-radius: 4px; | ||
padding: 12px; | ||
|
||
::placeholder{ | ||
color: #E7E7E7; | ||
} | ||
} | ||
.tags-container{ | ||
display: flex; | ||
gap: 12px; | ||
justify-content: space-around; | ||
align-items: center; | ||
|
||
li{ | ||
color: #0064c8; | ||
background-color: #F2F7FC; | ||
padding: 6px 30px; | ||
font-size: 16px; | ||
border-radius: 4px; | ||
cursor: pointer; | ||
} | ||
|
||
.active{ | ||
background-color: #0064c8; | ||
color: #FFF; | ||
} | ||
|
||
} | ||
|
||
} |
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.
🧹 Nitpick (assertive)
Melhore a acessibilidade da barra de pesquisa
A implementação da barra de pesquisa está boa, mas pode ser melhorada com:
- Labels para acessibilidade
- Feedback visual mais claro para estados de foco
- Cores com melhor contraste
.search-container {
input {
+ &:focus {
+ outline: 2px solid #0064c8;
+ border-color: transparent;
+ }
}
.tags-container {
li {
+ &:focus-visible {
+ outline: 2px solid #0064c8;
+ outline-offset: 2px;
+ }
}
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
.search-container { | |
width: 600px; | |
display: flex; | |
flex-direction: column; | |
margin: -1rem auto 2rem auto; | |
border-radius: 4px; | |
gap: 12px; | |
input{ | |
width: inherit; | |
border: 1.5px solid #E7E7E7; | |
border-radius: 4px; | |
padding: 12px; | |
::placeholder{ | |
color: #E7E7E7; | |
} | |
} | |
.tags-container{ | |
display: flex; | |
gap: 12px; | |
justify-content: space-around; | |
align-items: center; | |
li{ | |
color: #0064c8; | |
background-color: #F2F7FC; | |
padding: 6px 30px; | |
font-size: 16px; | |
border-radius: 4px; | |
cursor: pointer; | |
} | |
.active{ | |
background-color: #0064c8; | |
color: #FFF; | |
} | |
} | |
} | |
.search-container { | |
width: 600px; | |
display: flex; | |
flex-direction: column; | |
margin: -1rem auto 2rem auto; | |
border-radius: 4px; | |
gap: 12px; | |
input{ | |
width: inherit; | |
border: 1.5px solid #E7E7E7; | |
border-radius: 4px; | |
padding: 12px; | |
&:focus { | |
outline: 2px solid #0064c8; | |
border-color: transparent; | |
} | |
::placeholder{ | |
color: #E7E7E7; | |
} | |
} | |
.tags-container{ | |
display: flex; | |
gap: 12px; | |
justify-content: space-around; | |
align-items: center; | |
li{ | |
color: #0064c8; | |
background-color: #F2F7FC; | |
padding: 6px 30px; | |
font-size: 16px; | |
border-radius: 4px; | |
cursor: pointer; | |
&:focus-visible { | |
outline: 2px solid #0064c8; | |
outline-offset: 2px; | |
} | |
} | |
.active{ | |
background-color: #0064c8; | |
color: #FFF; | |
} | |
} | |
} |
.news-container { | ||
grid-area: news; | ||
|
||
h2 { | ||
display: flex; | ||
justify-content: center; | ||
margin-top: 40px; | ||
margin-bottom: 5rem; | ||
} | ||
|
||
.news-cards { | ||
@apply flex flex-wrap justify-center md:mt-10; | ||
|
||
.news-item { | ||
@apply flex flex-col max-w-sm; | ||
margin: 0 2.5rem 2.5rem 2.5rem; | ||
width: 20.125rem; | ||
gap: 6px; | ||
|
||
.text-container { | ||
display: flex; | ||
flex-direction: column; | ||
gap: 6px; | ||
|
||
.news-date { | ||
font-size: 12px; | ||
font-weight: 400; | ||
color: #66a2de; | ||
line-height: 24px; | ||
} | ||
} | ||
|
||
img { | ||
border-radius: 8px; | ||
width: 100%; | ||
height: auto; | ||
object-fit: cover; | ||
max-height: 238px; | ||
} | ||
|
||
h2 { | ||
display: flex; | ||
justify-content: center; | ||
} | ||
|
||
a.link { | ||
display: flex; | ||
align-items: center; | ||
margin: auto 0 1.5rem 1.5rem; | ||
|
||
button { | ||
gap: 1rem; | ||
|
||
p { | ||
font-family: "Work Sans"; | ||
font-size: 1rem; | ||
font-style: normal; | ||
font-weight: 500; | ||
line-height: 1.125rem; | ||
} | ||
|
||
svg { | ||
width: 1.5rem; | ||
height: 1.5rem; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
🧹 Nitpick (assertive)
Boa implementação do grid de notícias!
A estrutura do grid de notícias está bem organizada e responsiva. Alguns pontos de melhoria:
- Considere usar CSS Grid para melhor controle do layout
- Adicione transições suaves nos elementos interativos
.news-container {
.news-cards {
@apply flex flex-wrap justify-center md:mt-10;
+ display: grid;
+ grid-template-columns: repeat(auto-fit, minmax(20.125rem, 1fr));
+ gap: 2.5rem;
.news-item {
+ transition: transform 0.2s ease;
+
+ &:hover {
+ transform: translateY(-5px);
+ }
}
}
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
.news-container { | |
grid-area: news; | |
h2 { | |
display: flex; | |
justify-content: center; | |
margin-top: 40px; | |
margin-bottom: 5rem; | |
} | |
.news-cards { | |
@apply flex flex-wrap justify-center md:mt-10; | |
.news-item { | |
@apply flex flex-col max-w-sm; | |
margin: 0 2.5rem 2.5rem 2.5rem; | |
width: 20.125rem; | |
gap: 6px; | |
.text-container { | |
display: flex; | |
flex-direction: column; | |
gap: 6px; | |
.news-date { | |
font-size: 12px; | |
font-weight: 400; | |
color: #66a2de; | |
line-height: 24px; | |
} | |
} | |
img { | |
border-radius: 8px; | |
width: 100%; | |
height: auto; | |
object-fit: cover; | |
max-height: 238px; | |
} | |
h2 { | |
display: flex; | |
justify-content: center; | |
} | |
a.link { | |
display: flex; | |
align-items: center; | |
margin: auto 0 1.5rem 1.5rem; | |
button { | |
gap: 1rem; | |
p { | |
font-family: "Work Sans"; | |
font-size: 1rem; | |
font-style: normal; | |
font-weight: 500; | |
line-height: 1.125rem; | |
} | |
svg { | |
width: 1.5rem; | |
height: 1.5rem; | |
} | |
} | |
} | |
} | |
} | |
} | |
.news-container { | |
grid-area: news; | |
h2 { | |
display: flex; | |
justify-content: center; | |
margin-top: 40px; | |
margin-bottom: 5rem; | |
} | |
.news-cards { | |
@apply flex flex-wrap justify-center md:mt-10; | |
display: grid; | |
grid-template-columns: repeat(auto-fit, minmax(20.125rem, 1fr)); | |
gap: 2.5rem; | |
.news-item { | |
@apply flex flex-col max-w-sm; | |
margin: 0 2.5rem 2.5rem 2.5rem; | |
width: 20.125rem; | |
gap: 6px; | |
transition: transform 0.2s ease; | |
&:hover { | |
transform: translateY(-5px); | |
} | |
.text-container { | |
display: flex; | |
flex-direction: column; | |
gap: 6px; | |
.news-date { | |
font-size: 12px; | |
font-weight: 400; | |
color: #66a2de; | |
line-height: 24px; | |
} | |
} | |
img { | |
border-radius: 8px; | |
width: 100%; | |
height: auto; | |
object-fit: cover; | |
max-height: 238px; | |
} | |
h2 { | |
display: flex; | |
justify-content: center; | |
} | |
a.link { | |
display: flex; | |
align-items: center; | |
margin: auto 0 1.5rem 1.5rem; | |
button { | |
gap: 1rem; | |
p { | |
font-family: "Work Sans"; | |
font-size: 1rem; | |
font-style: normal; | |
font-weight: 500; | |
line-height: 1.125rem; | |
} | |
svg { | |
width: 1.5rem; | |
height: 1.5rem; | |
} | |
} | |
} | |
} | |
} | |
} |
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.
concordo com o bot tbm! css grid é bem mais simples de manter e de tornar responsível
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.
@zoedsoupe Obrigado por confirmar! Sim, o CSS Grid oferece um controle mais preciso sobre o layout e torna o código mais limpo e fácil de manter. A sugestão anterior também incluiu transições suaves para melhorar a experiência do usuário.
(`・ω・´)ゞ
✏️ Learnings added
Learnt from: zoedsoupe
PR: peapescarte/pescarte-plataforma#253
File: assets/css/noticias.scss:238-307
Timestamp: 2025-01-19T18:26:43.978Z
Learning: CSS Grid is preferred over flexbox for grid layouts in the Pescarte platform due to its better maintainability and responsiveness capabilities.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
@@ -4,7 +4,7 @@ | |||
export DATABASE_USER=postgres | |||
export DATABASE_PASS=postgres | |||
export DATABASE_HOST=127.0.0.1 | |||
export DATABASE_NAME=postgres | |||
export DATABASE_NAME=pescarte |
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.
isso aqui vai quebrar todo mundo... se precisar os valores, cria um .env.dev.local e sobrescreve as que vc precisa mudar nesse arquivo por favor
@@ -158,7 +174,7 @@ | |||
} | |||
|
|||
.noticia .noticia-descricao h2 { | |||
color: #0064C8; | |||
color: #0064c8; |
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.
o bot tá certo kk a gente um arquivo com todas as constantes de cores pra css, por favor, prefira usar elas doq literais de hexadecimal
.news-container { | ||
grid-area: news; | ||
|
||
h2 { | ||
display: flex; | ||
justify-content: center; | ||
margin-top: 40px; | ||
margin-bottom: 5rem; | ||
} | ||
|
||
.news-cards { | ||
@apply flex flex-wrap justify-center md:mt-10; | ||
|
||
.news-item { | ||
@apply flex flex-col max-w-sm; | ||
margin: 0 2.5rem 2.5rem 2.5rem; | ||
width: 20.125rem; | ||
gap: 6px; | ||
|
||
.text-container { | ||
display: flex; | ||
flex-direction: column; | ||
gap: 6px; | ||
|
||
.news-date { | ||
font-size: 12px; | ||
font-weight: 400; | ||
color: #66a2de; | ||
line-height: 24px; | ||
} | ||
} | ||
|
||
img { | ||
border-radius: 8px; | ||
width: 100%; | ||
height: auto; | ||
object-fit: cover; | ||
max-height: 238px; | ||
} | ||
|
||
h2 { | ||
display: flex; | ||
justify-content: center; | ||
} | ||
|
||
a.link { | ||
display: flex; | ||
align-items: center; | ||
margin: auto 0 1.5rem 1.5rem; | ||
|
||
button { | ||
gap: 1rem; | ||
|
||
p { | ||
font-family: "Work Sans"; | ||
font-size: 1rem; | ||
font-style: normal; | ||
font-weight: 500; | ||
line-height: 1.125rem; | ||
} | ||
|
||
svg { | ||
width: 1.5rem; | ||
height: 1.5rem; | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} |
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.
concordo com o bot tbm! css grid é bem mais simples de manter e de tornar responsível
@@ -1,4 +1,4 @@ | |||
version: "3.8" |
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.
por que essa linha foi removida? isso pode causar inconsistências em versões diferentes do docker compose
lib/pescarte/blog/blog.ex
Outdated
@@ -15,7 +16,7 @@ defmodule Pescarte.Blog do | |||
optional(:page_size) => pos_integer() | |||
} | |||
|
|||
@spec list_posts_with_filter(filters()) :: {:ok, list(Post.t())} | {:error, term()} | |||
@spec list_posts_with_filter(filters()) :: list(Post.t()) | {:error, term()} |
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.
além de concordar com o bot, vou mais além: por que a spec do retorno foi alterado? esse retorno não é semântico e pode mais confundir doq ajudar
ou retorna apenas uma lista (q pode ser vazia ou não) ou retorna as tuplas de sucesso e erro, considerando q essa função pode gerar outros erros para além do banco de dados
|
||
def show(conn, _params) do | ||
current_path = conn.request_path | ||
render(conn, :show, current_path: current_path, error_message: nil) | ||
|
||
[main_new | news] = Blog.list_posts_with_filter() |
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.
a sugestão de refactor do bot foi bem meia boca kk mas é isso, use pattern matching pq quebrar ai vai impedir a renderização da pagina e portando retornar 500 pro usuário, oq n é legal (:
def handle_event("get_news", _, socket) do | ||
current_news_length = socket.assigns.news.length | ||
loaded_news = Blog.list_posts_with_filter(%{page: current_news_length + 1, page_size: 6}) | ||
|
||
socket = socket |> assign(news: socket.assigns.news ++ loaded_news) | ||
|
||
{:noreply, socket} | ||
end |
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.
talvez valha a pena de vermos de usar stream pra listagem de notícias, pq com o tempo vai aumentar bastante o número de notícias em memória, mas n precisa ser agora, apenas um lembrete
vou tentar rodar sua branch aqui e ver oq pode tá rolando |
Descrição
PR para mudar a estilização das páginas relacionadas as notícias e implementar a utilização dos filtros.
Pontos para atenção
mount/3
também funciona, o problema realmente estar no uso dophx-click
.Possui novas configurações?
Summary by CodeRabbit
Notas de Lançamento
Novas Funcionalidades
Melhorias de Interface
Configurações
Ajustes Técnicos