-
Notifications
You must be signed in to change notification settings - Fork 0
My Studies Page #134
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
base: main
Are you sure you want to change the base?
My Studies Page #134
Conversation
</div> | ||
<!-- STUDY TABLE --> | ||
<div class="rounded-xl border border-neutral-300 overflow-y-auto"> | ||
<!-- TABLE HEADERS --> |
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.
There's an issue with clipping and responsiveness when screen is small I think due to the way the studies items are being handled within the MyStudiesItem component and the table headers in the MyStudiesPage. Not immediately noticeable. will fix when I figure out how
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 there's a large margin being applied to the entire my studies page that cuts off study items when the screen becomes small
</div> | ||
|
||
<div class="space-y-6"> | ||
<!-- RECENT STUDIES --> |
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.
should figure out how to show recent studies if no studies have been made yet
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.
Really good work on the my studies page! It looks good but I think it needs some changes before we merge. Also I'm not sure why the prettier and lint checks failed because I don't see any formatting issues so maybe delete this PR and we can put up a new PR when changes are made. Let me know if you have any questions about my comments or if you want to work on the ticket together!
<template> | ||
<el-button | ||
class="bg-primary-300 text-white border border-primary-400 rounded-lg px-5 py-1 justify-center" | ||
class="bg-primary-200 text-neutral-10 border border-primary-300 rounded-lg px-5 py-1 justify-center" |
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.
We should check with Christine to confirm the styling of the button
</div> | ||
<!-- STUDY TABLE --> | ||
<div class="rounded-xl border border-neutral-300 overflow-y-auto"> | ||
<!-- TABLE HEADERS --> |
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 there's a large margin being applied to the entire my studies page that cuts off study items when the screen becomes small
</div> | ||
</div> | ||
<!-- STUDY TABLE --> | ||
<div class="rounded-xl border border-neutral-300 overflow-y-auto"> |
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 using div for the table, could look into using HTML tables (https://www.w3schools.com/html/html_tables.asp) or Element Plus tables (https://element-plus.org/en-US/component/table.html#table)
</div> | ||
</RouterLink> | ||
|
||
<!-- <div class="flex border-b border-neutral-300 py-6 items-center gap-6"> |
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 commented code?
:to="{ name: 'study', params: { id } }" | ||
class="block hover:bg-neutral-50 transition-colors" | ||
> | ||
<div |
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.
There's a lot of divs here, so I'm wondering if it could be simplified after switching to using an HTML or Element Plus table to list all the studies. Maybe we could get rid of MyStudiesItem and change it to MyStudiesTable so all studies are contained in one component
:to="{ name: 'study', params: { id } }" | ||
class="block hover:bg-neutral-50 transition-colors" | ||
> | ||
<div |
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 use Element Plus Card component (https://element-plus.org/en-US/component/card.html#card) instead of a div for each item. Also seems like there are a lot of nested divs here, can look into simplifying the HTML
UI update