-
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
Justinxue/phs 49 exhibit page #45
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.
pretty good! a few things to take a look at, but overall nice work @jxmoose !
.github/workflows/lint.yml
Outdated
# # run: | | ||
# # # Get list of staged TypeScript files | ||
# # files=$(git diff --cached --name-only --diff-filter=d | grep '\.tsx\?$') | ||
################################ |
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.
is this accurately needed in this pr? this seems like some linting changes that should be in a different pr, but if it's necessary for this pr, thats fine.
database.types.ts
Outdated
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.
empty file being added, seems like it might not need to be here
src/app/exhibitsPage/page.tsx
Outdated
// ignore the native anchor action | ||
evt.preventDefault(); | ||
|
||
window.history.back(); |
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.
edge case that can likely be dealt with later: what if this is the first page we end up on? (i.e. if we open up a new tab and go straight to the exhibits page, what should we see?)
id: string; | ||
}) { | ||
return ( | ||
<ul key={id}> |
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.
you might have meant a
the normal syntax of an unordered list:
<ul>
<li></li>
<li></li>
<li></li>
</ul>
* @param root0.id id of exhibit | ||
* @returns exhibit component | ||
*/ | ||
export default function Exhibit({ |
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 is an example of something that could be refactored to match both the web and mobile screen versions, but this is something we can talk about later @sarahhpeng @jxmoose
<div className="w-[100%] px-4 pt-6 pb-7 bg-mint-cream rounded flex-col justify-start items-start gap-2.5 inline-flex mt-6"> | ||
<div className="flex-col justify-start items-start gap-5 flex"> | ||
<div className="justify-start items-center gap-2 inline-flex"> | ||
<div className="text-neutral-700 text-lg font-bold font-['Lato']"> |
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.
text styling should likely be done with <h_> tags (in this case h2), since it is based off the H2 text style in the design. i haven't looked through the way styling is done in the app though, so this might be wrong.
{title} | ||
</div> | ||
</div> | ||
<div className="text-black text-base font-light font-['Lato']"> |
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.
same concept here, with a p tag.
src/types/supabase.ts
Outdated
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.
check that this is accurate, but this seems fine to me?
src/types/supabase.ts
Outdated
Relationships: [] | ||
} | ||
} | ||
category?: Database['public']['Enums']['tour_category']; |
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.
style change mostly in this file, check if this passes prettier
DEZIN COMMENTS (pls finish latest EOD 4/10 and text me when done so I can review) @jxmoose
|
627d435
to
1ba952e
Compare
made big database changes. basically divided exhibits into categories (which hold all the info on exhibits), and the exhibits table (which holds the locations, and an id pointing to which category it is). this allowed all the fun facts to essentially be the same on the map. |
src/app/exhibitsPage/page.tsx
Outdated
<div className="flex-col justify-start items-start mt-2"> | ||
<div className="text-night text-[32px] font-bold leading-9 font-['Lato'] mb-4"> | ||
Our Exhibits{' '} | ||
</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.
Use a text based tag (p, h1, h2, etc..) for abstraction purposes. Use inspect on figma file to choose the right tag, review globals.css file for custom aliases. Swithc text-[32px] to tailwind units -> text-8 but there is probably a tag in globals.css that has the right styling
src/app/exhibitsPage/page.tsx
Outdated
</div> | ||
<Link href="/siteMapPage"> | ||
<div className="px-4 py-2 mb-2 mt-6 rounded-md border active:border-hunterGreen border-asparagus justify-start items-start inline-flex"> | ||
<div className="active:text-hunterGreen text-center text-asparagus text-base font-bold font-['Lato'] leading-tight"> |
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.
Use a text tag. See above comment / talk to Andrei.
src/app/exhibitsPage/page.tsx
Outdated
<div className="text-night text-base leading-5 font-normal font-['Lato']"> | ||
Saratoga is home to an abundance of plant and animal life. As you | ||
explore these exhibits you will learn about species that are | ||
endangered and being carefully monitored by scientists with |
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.
text tag, see above comment
<div className="w-[100%] px-4 py-8 bg-mint-cream rounded-lg flex-col justify-start items-start gap-2.5 inline-flex mt-6"> | ||
<div className="flex-col justify-start items-start gap-5 flex"> | ||
<div className="justify-start items-center gap-2 inline-flex"> | ||
<h2 className="text-night text-2xl font-semibold font-['Lato']"> |
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.
some of this styling isn't necessary. See globals.css to see the default styling on h2 tags. we do this to reduce redundant styling.
}) { | ||
return ( | ||
<li key={id} id={`a${id}`}> | ||
<div className="w-[100%] px-4 py-8 bg-mint-cream rounded-lg flex-col justify-start items-start gap-2.5 inline-flex mt-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.
w-full instead of w-[100%] for convention.
@@ -130,7 +129,7 @@ function DisplayPreviewCard({ | |||
/> | |||
</svg> | |||
</div> | |||
<Link href={`/spotlightPage/${id}`}> | |||
<Link href={`/exhibitsPage/#a${id}`}> | |||
<h3 className="relative truncate text-asparagus pr-[0.31rem] pl-[0.75rem] pt-[0rem] uppercase font-light text-xs leading-normal"> |
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.
check globals.css to avoid redundant styling. see above comments
src/app/exhibitsPage/page.tsx
Outdated
import Exhibit from '../../components/userComponents/Exhibit/Exhibit'; | ||
import BackButton from '../../components/userComponents/BackButton/page'; | ||
|
||
/** |
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.
More comprehensive comments for this page and what's happening in useEffect. Assume minimal background knowledge of this sprint for handoff
5cd000c
to
2a08d25
Compare
✨ New in this PR ✨
One liner: what did you do? 🛠
Added exhibits database, queries, and page
Coverage 🙆♀️
use this section to break up your task into submodules and track progress, copy from Linear and mark what was completed
How can the reviewer test your code? Where is this sprint located in the codebase? 👩💻
src/components/userComponents/Exhibit/Exhibit.tsx
src/app/exhibitsPage/page.tsx
REQUIRED: Link a short video demo of the feature you have added. 👩💻
https://www.loom.com/share/6d3149a17f844be3a3b750075c30b48d?sid=a0b835d9-146b-4b21-abd3-a4c87ddc0e5f
Any bugs you encountered or still having trouble with? 🐛
Resources 📔
REQUIREMENTS in this section: Link all relevant online resources and related PRs. Write down any new dependencies or packages and link to their documentation. If you ran into bugs, link any stack overflow or online resources that helped you unblock yourself
❗IF this sprint was marked as FRONTEND on linear, cc Claire Wang
@cluurie
REMEMBER: once PR is made, also request a PR review Sarah, CC'ing is NOT enough!
🏚 cc: @sarahhpeng @andreisito