-
Notifications
You must be signed in to change notification settings - Fork 69
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
Mejora de UI/UX #764
base: develop
Are you sure you want to change the base?
Mejora de UI/UX #764
Conversation
El primer commit es solo la configuración del archicvo docker-compose.yml. Hice commit y se me olvidó borrarlo. Un saludo! |
Hello Guille, first of all, welcome and thank you for wanting to contribute! It's great to see you here 😉!
About the code I'll do a fast review now, but I'd like to check it in more depth later. |
Bienvenido @gmartincor! Encantados de tener más ayuda en este proyecto 💪 Algunas notas:
Saludos 👋👋 |
Hello!! Thanks so much for your time and attention. I'll get right on those corrections. 😁 Always a pleasure! Best regards!! 👋👋 |
Hello @gmartincor I think we should also revert the second point:
Not sure what problem are you trying to solve here, IMO the current default ordering and sorting is fine (it took us some time to reach the current behavior, see this thread #495 for more information). |
Hello @markets , I thought it would be a good idea for administrators to appear first by default in the user list when opening the site for the first time, so that the rest of the members can have easier and more intuitive visual access to them, while maintaining the order based on the last connection for both administrators and the rest of the members. |
Sorry @gmartincor 🙏🏼 but I don't agree with that. IMO we should revert that change. In general, it's a good idea to use each branch/PR for one single thing, that's the easiest way to move forward. I think we should use this branch to fix only this issue #729. And if we want to change the default ordering, it will be better to discuss it before and in another thread. Focused branches are the best 👌🏼 for everybody (developer, reviewer, tester, releaser, product). |
Hey @gmartincor let's do it as @markets said and focus only on that issue. Nevertheless, I'm going to take the opportunity to give you a few more tips. |
Hello 👋 @gmartincor Welcome to the wonderful world of open source! It helped me a lot to grow as a developer and I still learn new things everyday after more than 10 years doing so 🙌 Hope we can help you too with those code reviews and discussions, they are always very useful. PS sorry 🙏 we are still using a very old Bootstrap version, I just created a new issue (#766) to migrate to a newer version some day |
"Hello @markets , yesss!! My intention is to collaborate while I continue learning and growing as a developer. Thank you very much for your support. I still don't have much experience with Bootstrap, but I would like to carry out the migration to the updated version if it is possible 😁. |
@gmartincor after all those changes, we're still not fixing the original issue (#729). Actually, the current diff in this branch:
I still think we should focus on fix/improve the issue described in #729. Small steps is the way to go. |
justify-content: space-between; | ||
align-items: stretch; | ||
gap: 20px; | ||
|
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.
remove this space
gap: 20px; | ||
|
||
|
||
@media(max-width: $screen-sm-min) { |
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.
I think this should be lg instead of sm
.card-wrapper { | ||
width: 48%; | ||
|
||
@media (max-width: $screen-sm-min) { |
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.
this should be lg as well
} | ||
} | ||
|
||
.card-wrapper { |
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.
Maybe to-member-card__wrapper
is a better name
flex-direction: column; | ||
justify-content: space-between; | ||
height: 100%; | ||
|
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.
remove this space
} | ||
|
||
&__description { | ||
margin-bottom: 10px; |
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.
I don't think we should remove this margin
width: 100%; | ||
} | ||
} | ||
|
||
.to-member-card { | ||
& { | ||
margin: 16px 0; |
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.
I think it's better if we remove this margins as they are having a strange effect
app/views/users/index.html.erb
Outdated
@@ -20,7 +20,7 @@ | |||
</div> | |||
</div> | |||
|
|||
<div class="row to-member-cards"> | |||
<div class="card-container to-member-cards"> |
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.
instead of creating card-container
class you could have given some styles to to-member-cards
Mejora de UI/UX: Optimización de la presentación y ordenamiento de miembros
Este Pull Request introduce dos posibles en la UI/UX de la aplicación:
Optimización del diseño con Flexbox (member-cards.scss)
Uso de Flexbox para las columnas: Se ha implementado
flex-wrap
para asegurar que las columnas se distribuyan en filas cuando sea necesario y que los elementos se ajusten uniformemente.Distribución vertical: Cada tarjeta (card) utiliza
flex-direction: column
para organizar su contenido verticalmente, junto conjustify-content: space-between
para distribuir el espacio de manera uniforme dentro de la tarjeta.Diseño responsive: En pantallas más pequeñas (móviles), los cards se expanden para ocupar todo el ancho disponible para mejorar la usabilidad en dispositivos móviles.
Reordenamiento de miembros en la aplicación (users_controller.rb)
Modificaciones en el método
index
: Se ha eliminado el orden predeterminado del métodoindex
, delegando el manejo de la ordenación al métodosearch_and_load_members
. Esto busca asegurar que los parámetros proporcionados por el usuario se procesen de forma flexible, sin imponer un orden preestablecido.Modificaciones en el método
search_and_load_members
:Uso de
joins(:user)
: Se ha añadido un JOIN entre las tablasmembers
yusers
para acceder a los campos del modeloUser
, comolast_sign_in_at
a través de Ransack. Esta modificación se implementa con la intención de que el sistema puede ordenar y filtrar tanto por atributos de members como de users, buscando una mayor flexibilidad en el ordenamientoCondicional para el orden predeterminado: Si el usuario no especifica ninguna orden de búsqueda (
result.order_values.blank?
), se aplica un orden predeterminado de los miembros: primero los administradores (manager DESC
), seguidos de los miembros por última conexión (last_sign_in_at DESC NULLS LAST
). Esta modificación busca mejorar la accesibilidad visual y hacer la interfaz más intuitiva, mostrando primero a los administradores y luego a los miembros de acuerdo a su última actividad. En el caso de que el usuario especifique un criterio de ordenación, este se respeta y se ajusta para asegurar su criterio de búsqueda.Espero que la contribución pueda ser de utilidad.
Un saludo!!