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

Cohorts section updated in index.tsx #35

Merged
merged 4 commits into from
Sep 18, 2024
Merged

Conversation

andrealbiac
Copy link
Contributor

Updated the Cohorts section replacing the table by a new image. This is my first code change PR (& also writing in a .tsx file) so I hope I didn't mess up the divs annotation or broke anything!

I think there's an extra div that we don't need now, that was probably centering the text column with the table or smth, and now that we don't have the table anymore it's probably redundant, but not sure. Also maybe now we need to adjust the width or height, I used the same parameters to similar images in other sections.

Deleted table:
Captura de pantalla 2024-09-17 a las 15 52 02

How it should look like now (don't mind the margins):
Cohorts section

This addresses changes that were discussed in #34

Updated the Cohorts section replacing the table by a new image. This is my first code change PR (& also writing in a .tsx file) so I hope I didn't mess up the divs annotation or broke anything!

I think there's an extra div that we don't need now, that was probably centering the text column with the table or smth, and now that we don't have the table anymore it's probably redundant, but not sure.
Copy link

vercel bot commented Sep 17, 2024

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 Sep 18, 2024 9:21am

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.

Nice Andrea creating a PR! 🙏🙏

I've done some tweaks:

  • Deleted all cohorts data logic, since it was not used anymore we were getting lint errors that wouldn't let us update Vercel build
  • Tweaked spacings/responsive. Here is the before/after video to check. Could you confirm if you see it OK in mac super high resolution?
  • Updated asset names so we don't get caught again by the code police officer :)

The question here is:

  • Are we sure we want to delete the cohort table?
  • Shouldn't we change the "Supporting up-and-coming high-impact devs" section, since we don't have the open developer streams anymore? Or it's in the works still?

Thanks for the PR Andrea! Love to see you tweaking code and managing PRs, you should install Cursor + AI if you haven't yet, it's a great helper for devs! 🙌

Copy link
Contributor

@carletex carletex left a comment

Choose a reason for hiding this comment

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

This looks good! Thanks @andrealbiac & @Pabl0cks

@carletex
Copy link
Contributor

Are we sure we want to delete the cohort table?

It looks like it, yes. "they are more like internal spending numbers"

Shouldn't we change the "Supporting up-and-coming high-impact devs" section, since we don't have the open developer streams anymore? Or it's in the works still?

Yes, I think the graphic is ready but we still need a new copy for the section. We can do it in another PR when ready-

Updated asset names so we don't get caught again by the code police officer :)

🤣

@Pabl0cks Pabl0cks merged commit 1cbcaa5 into main Sep 18, 2024
3 checks passed
@Pabl0cks Pabl0cks deleted the andrealbiac-cohorts-update branch September 18, 2024 09:46
@Pabl0cks
Copy link
Member

👌 great! Merging! We can tweak in future PR if it doesn't look great in mac.

@andrealbiac
Copy link
Contributor Author

andrealbiac commented Sep 18, 2024

Hey!

  • Could you confirm if you see it OK in mac super high resolution?

Yes it looks great on mac as well, thanks @Pabl0cks !

And thanks for the support as well 🫶🏻 it's exciting! Gonna fork the repo so it'll works next time, and I'll download Cursor for sure! Will hit you up if I have more doubts :)

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.

3 participants