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

Refine layout and wording on page #63

Merged
merged 10 commits into from
Jan 7, 2025

Conversation

phipsae
Copy link
Contributor

@phipsae phipsae commented Jan 4, 2025

Description

Hi,
I've updated the layout and added more text to provide a clearer explanation of what we're doing (see the screenshots below).
I hope that's okay!

Thanks @Pabl0cks it is already a nice page! Open for feedback :)

Screenshot 2025-01-04 at 14 09 47 Screenshot 2025-01-04 at 14 09 56 Screenshot 2025-01-04 at 14 10 01

Copy link

vercel bot commented Jan 4, 2025

@phipsae is attempting to deploy a commit to the BuidlGuidl Team on Vercel.

A member of the Team first needs to authorize it.

Copy link

vercel bot commented Jan 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
buidlguidl-landing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 6:42am

Copy link
Member

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the tweaks! I like the new layout and content much more, just a few comments:

  • "What are BuidlGuidl Batches?" sectopm could use a bit more spacing, at least in my laptop everything feels too tight. Mobile layout needs a review too.
  • "What you'll learn?" section looks good to me!
  • "How do Batches work?" something feels strange to me with this card layout, maybe that's a personal perception. I think we could use a bit extra spacing here too.

If needed we could ask Andrea, if she's available, to do a final review or give the designer touch.

@phipsae
Copy link
Contributor Author

phipsae commented Jan 6, 2025

Thanks for the fast feedback! :)

I fixed the mobile view (thanks for pointing out), added some more spacing and created two components.
What do you think of creating more components to make the code more readable?

For the "How do Batches work?" I wanted to have another card layout that doesn't repeat that from the "What are BuidlGuidl Batches?". For me it seems good, but open to change it :)

If Andrea has time that would be nice! :)
Could you please ask her?

@phipsae phipsae requested a review from Pabl0cks January 6, 2025 08:42
@Pabl0cks
Copy link
Member

Pabl0cks commented Jan 6, 2025

For the "How do Batches work?" I wanted to have another card layout that doesn't repeat that from the "What are BuidlGuidl Batches?". For me it seems good, but open to change it :)

Yeah I think is good enough! The pills felt a bit strange at start but it's fine! Everything looking good now, just a couple of nitpicks:

  • Can you give more line spacing to the card titles here?

    Screenshot

    image

  • What do you think about changing the order of the table and the CTA from the last section? Having already a CTA at top, I think visually it would look a bit better for the whole section. Ignore if you feel like it's better like it is 🙏

    Screenshot

    image

@phipsae
Copy link
Contributor Author

phipsae commented Jan 6, 2025

I added the spacing, definitely better! (Funny that I didn’t notice it earlier 😅)

As for the order of the CTA and the table, I tried moving the CTA to the bottom, but I didn’t like it much. I’m happy with how it is now, but if you want to change it, let me know!

@@ -172,28 +267,7 @@ const Batches = ({ batchData, openBatchNumber, openBatchStartDate }: PageProps)
<div className="bg-[#EDEFFF] pt-16 lg:pt-24 pb-16">
Copy link
Member

Choose a reason for hiding this comment

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

Maybe with pt-2 lg:pt-8 is enough spacing?

Suggested change
<div className="bg-[#EDEFFF] pt-16 lg:pt-24 pb-16">
<div className="bg-[#EDEFFF] pt-2 lg:pt-8 pb-16">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link
Member

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

LGTM to merge!

Added a small comment about the spacing from the last CTA to the above section, but can leave it as it is or tweak it however you see it better!

In the future if we feel the cards font size for the first section is too big, could lower it a bit 🙏

@Pabl0cks
Copy link
Member

Pabl0cks commented Jan 7, 2025

Just realized vercel build was failing, tried to yarn build in local and got this:

Build optimization failed: found pages without a React Component as default ex
port in
pages/batches/components/BatchCta
pages/batches/utils/formatDate
pages/batches/components/Card

See https://nextjs.org/docs/messages/page-without-valid-component for more info.

@phipsae think we need to move the new components to /components and the new util to utils

@phipsae
Copy link
Contributor Author

phipsae commented Jan 7, 2025

Thx!
Fixed it. Now its successfully deployed.
Now we only need to promote it to production :)

@Pabl0cks Pabl0cks merged commit a83ad60 into BuidlGuidl:main Jan 7, 2025
3 checks passed
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