Add Sections Component #49
Merged
j285he
merged 12 commits from j285he-sections into main 9 months ago
snedadah
e26chiu
b72zhou
a258wang
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'j285he-sections'
Deleting a branch is permanent. It CANNOT be undone. Continue?
Closes #43.
Will look better when !46 is merged in.
https://j285he-sections-csc-class-profile-staging-snedadah.k8s.csclub.cloud/playground/
export default function Sections({ data }: SectionsProps) {return (<section className={styles.sections}><h1>Sections</h1>It would be super useful if we add a prop like
showHeader, and if it is false, it will not show the header or the separator. That way we will be able to reuse this component for the nav bar, which has the exact same list but a different header.{data.map((datum, index) => {return (<div key={`${datum.name}-${index}`} className={styles.navItem}><h4>{String(index).padStart(2, "0")}</h4>I wonder if we should move the number inside the a tag below so it's also clickable?
I think having the numbers be clickable might be nice!
data: SectionsData[];}export default function Sections({ data }: SectionsProps) {We should change this to a non-default export "export function Sections" since thats the convention we are following.
.sections {display: flex;flex-direction: row;width: 50%;This might not look great on smaller screens - maybe just try setting a reasonable
max-width?did you mean
min-width? I tried out max-width but it did not look too good on smaller screens.Hmm, I was playing around with the browser dev tools, and maybe we don't even need to set the min/max width at all and it'll still look the same?
I think we can just style the component separately for mobile screens, but that can wait for a later date.
display: flex;flex-direction: row;width: 50%;gap: 15px;NIT:
calc(15rem / 16)display: flex;flex-direction: row;align-items: flex-start;margin: calc(20rem / 16);There's appears to be a larger space between the "Sections" header and the separator vs. the nav items and the separator. Adding
margin-left: 0should be one way to make the spaces equally sized.}.nav a:hover {color: var(--primary-accent-light);NIT: I personally am not a big fan of the colour change on hover - I think the underline and pointer cursor are sufficient hover effects. Up to you though. :)
(Getting rid of the colour change would also be helpful if we make the numbers clickable as well.)
export const sectionsData = [NIT: maybe name this file
sections.ts? I guessroutes.tsalso makes sense though.Was thinking that the bottom nav component might need a list of all pages so we could just use this as a single source of truth
Note: After moving the component out of the playground's
<h2>tag (which was a mistake), I think the alignment of the separator, the middle of the h1, and the middle of the first anchor is slightly unaligned. i will change it when the text fixes are doneAdded some more small comments. I'm going to wait until after #46 is merged and the padding is fixed to see what this looks like before approving.
.sections {display: flex;flex-direction: row;min-width: calc(1000rem / 16);Continuing from thoughts here: #49
Maybe try getting rid of min-width and see what that looks like? I suspect it'll be fine if we don't specify the width at all.
Removed min-width, still doesn't look correct on small screens but might be better when we get mobile-specific font sizes?
<div key={`${datum.name}-${index}`} className={styles.navItem}><a href={datum.url}><h4>{String(index).padStart(2, "0")} </h4><p>{datum.name}</p>Just looking at this markup more closely, some thoughts:
h4andpinsidea? Maybe we can get rid of theh4andp, then use spans to style specific text.as inlis instead ofdivs, and throw them in anolorulinside of the nav. So the nesting would be likeI changed the heights, do I need a re-approval @a258wang ?
8253f6cbabinto main 9 months agoReviewers
8253f6cbab.