-
Notifications
You must be signed in to change notification settings - Fork 2
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/cgd 34 #12
Feature/cgd 34 #12
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Initial review, it looks good. However, can you rename the my-team folder to directory and also the MyTeamPage to DirectoryPage. It may sound silly, but this is just to keep it consistent with the naming in the designs. |
@Dan-Y-Ko Done! |
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.
Can you update the avg hr / sprint column. There has been an update to the design. https://www.figma.com/file/OAKUcYBLP3UOaRlnKrcf1G/Chingu-Dashboard?type=design&node-id=2038-9408&mode=dev
Don't worry about the color changes, that can be taken care of in a different pr. Make sure to also take care of the placeholder text and the "user edited" value. Don't worry about the modal though.
@Dan-Y-Ko I think it's done 🤓 |
@Dan-Y-Ko Not sure about Edit Button's hover effect, looks a bit weird. |
I agree. However, I think we can take care of the small details in separate prs. I don't want to hold up people's prs over every little thing. We'd never close any prs if we did that lol. I will take note (or try to anyways) of things like this and resolve it with Eury and then either myself or someone else can fix it in a separate pr. Just to make sure, is this what you see too: https://i.imgur.com/jkjGLgn.png |
Yeah, this grey background. I can remove this hover effect by adding additional classes, maybe I can add some icon's color change on hover instead or I can just change the edit button a bit: |
Eury responded saying there isn’t a hover effect so I think we can go without one. However, I do like how it looks in your 2nd screenshot |
Should I just change this hover effect (remove grey background)? I can add some custom classes, I think this bg color is coming from DaisyUI. |
@Dan-Y-Ko Done! |
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.
lgtm!
Description and impacts
Jira related story
CGD-34
Screenshots