-
Notifications
You must be signed in to change notification settings - Fork 1
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
[FE] creating obtion button #156
Conversation
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.
Muy prolijo! Dejo un par de comentarios pero ya te pre apruebo
|
||
type MemberCardProp = { | ||
member: Member; | ||
type: 'Buttons' | 'Tags'; | ||
fetchData?: () => void; | ||
// eslint-disable-next-line no-unused-vars | ||
onError?: (error: string[]) => void; | ||
imAdmin?: boolean; |
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 entiendo el imAdmin. Osea significa que el user loggeado es admin o que el member particular que esta viendo es admin? Creo que quedaria mas claro con isAdmin
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.
El imAdmin la puse para que si cuando se renderizan las member cards si el usuario que esta viendo el grupo es admin, le muestra en todas las member cards el botoncito de opciones
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 deberia ser isAdmin?
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.
ahi cambie @santimintegui @Mr-T-Smyth
// look if i am an admin | ||
setImAdmin( | ||
getMembers.findIndex( | ||
(member) => |
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.
Aca solo pones true si es un admin y es el mismo, entonces el chequeo que haces adentro del componente a ver si es el mismo es redundante no?
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.
Jajaja como no queria fijarme si habia un endpoint o algo que me dijera si soy admin hice esa logica ahi.
Hay dos puntos a resaltar en la logica de mostrar el boton de opciones:
- Si yo soy admin del grupo me muestran todos los botones
- Para la member card que es mia, no se mostraria el boton, tipo no me podria sacarme a mi el rol de admin, y si me quiero salir del grupo tengo el boton de abajo en realidad
|
||
type MemberCardProp = { | ||
member: Member; | ||
type: 'Buttons' | 'Tags'; | ||
fetchData?: () => void; | ||
// eslint-disable-next-line no-unused-vars | ||
onError?: (error: string[]) => void; | ||
imAdmin?: boolean; |
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 deberia ser isAdmin?
<div className='absolute z-20'> | ||
<div className='z-20 float-left'> | ||
<Menu.Items className='fixed right-0 z-20 mt-2 block w-56 origin-top-right rounded-md bg-white py-1 shadow-lg ring-1 ring-black ring-opacity-5 focus:outline-none lg:right-auto'> | ||
{role === 'participant' && ( |
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.
es posible juntar estos dos button en uno solo y cambiar el label de administrador por miembro?
algo asi se me ocurre :
<Menu.Item> <button className='group flex w-full items-center rounded-md px-2 py-2 text-sm text-gray-900'> Designar como ${role === 'participant' ? 'miembro' : 'administrador'} </button> </Menu.Item>
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.
Seee, ya lo cambio
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.
Ahi lo cambie @santimintegui
What && Why
Ahora podes simular que como admin podes hacer cosas sobre los miembros!
El cambio es meramente visual, despliega las opciones cuando sos admin
Links
Ticket en trello
Screenshots