Skip to content
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

Initialize footer #20

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Initialize footer #20

wants to merge 5 commits into from

Conversation

hetd54
Copy link
Collaborator

@hetd54 hetd54 commented Oct 22, 2024

No description provided.

Copy link

github-actions bot commented Oct 22, 2024

Visit the preview URL for this PR (updated for commit dc8e504):

https://ccv-website-next--pr20-footer-4ew5pfi2.web.app

(expires Wed, 13 Nov 2024 21:29:13 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 809aa2d3a6cfe026c3c7417c932015a8f1d9ec89

@hetd54 hetd54 marked this pull request as ready for review November 6, 2024 21:28
@hetd54 hetd54 self-assigned this Nov 6, 2024
Copy link
Collaborator

@anna-murphy anna-murphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty good~ the only thing id flag is maybe a md style on the footer links, but other than that i think you're okay to merge 🌠

<div className="mt-8 grid grid-cols-1 lg:grid-cols-5 gap-8 xl:col-span-2">
{Object.entries(navigation).map(([_, contents]) => {
return (
<div key={_}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't love using the _ here, it's a little nitpicky, but if you're going to use the value I think you should either change the _ to key or something or use the href for the key value. So it could look like either of these:

  • <div key={key> ...
  • <div key={href}> ...

@@ -9,7 +13,7 @@ const LogoBrown: React.FC = () => {
xmlns="http://www.w3.org/2000/svg"
x="0px"
y="0px"
width="100%"
width={width}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this pattern!

1fr
min(1000px, 100%)
1fr;
@apply bg-white;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happened to the styles that got removed here? that's intentional?

</div>
</div>

<div className="mt-8 grid grid-cols-1 lg:grid-cols-5 gap-8 xl:col-span-2">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i was checking this out on the preview and maybe we should have a md style here too? like maybe md:grid-cols-3 or something? it's a little awkward in the sm to lg transition period i think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants