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

feat: add hero component #13

Merged
merged 21 commits into from
Oct 4, 2024
Merged

feat: add hero component #13

merged 21 commits into from
Oct 4, 2024

Conversation

selina-kim
Copy link
Contributor

No description provided.

@selina-kim selina-kim self-assigned this Sep 20, 2024
Copy link

netlify bot commented Sep 20, 2024

Deploy Preview for deltahacks11 ready!

Name Link
🔨 Latest commit a22630d
🔍 Latest deploy log https://app.netlify.com/sites/deltahacks11/deploys/66ff6d4175dc4a000803e471
😎 Deploy Preview https://deploy-preview-13--deltahacks11.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@demanr demanr linked an issue Sep 29, 2024 that may be closed by this pull request
@Krish120003
Copy link
Member

The fire animation gif is highly resource intensive and causes lag.

@Krish120003
Copy link
Member

The MLH Logo is too bold.

@Krish120003
Copy link
Member

The header font does not match Figma

@Krish120003 Krish120003 marked this pull request as ready for review October 3, 2024 03:09
Krish120003
Krish120003 previously approved these changes Oct 3, 2024
Copy link
Member

@Krish120003 Krish120003 left a comment

Choose a reason for hiding this comment

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

There's a lot of performance work that we could do, and we should avoid using px values as definitions and stick to em's for sizing. However, this is a functional hero and looks good for the initial release due to short deadlines.

src/sections/Hero.tsx Outdated Show resolved Hide resolved
src/sections/Hero.tsx Show resolved Hide resolved
@Krish120003 Krish120003 requested review from arian81 and kabirsg October 3, 2024 03:13
@demanr demanr self-requested a review October 3, 2024 03:17
demanr
demanr previously approved these changes Oct 3, 2024
Copy link
Contributor

@demanr demanr left a comment

Choose a reason for hiding this comment

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

Seems legit, small cleaning to do but functionality complete

@kabirsg
Copy link
Contributor

kabirsg commented Oct 3, 2024

image
sometimes on medium sized (ipads - pic is from iPad mini) screens the white text gets in front of the light purple mountains making it a little hard to read. If we are ok with this then can ignore but just wanted to bring it up.

@Krish120003
Copy link
Member

image sometimes on medium sized (ipads - pic is from iPad mini) screens the white text gets in front of the light purple mountains making it a little hard to read. If we are ok with this then can ignore but just wanted to bring it up.

@Shivermist is this ok? How should this be handeled?

@Shivermist
Copy link

image sometimes on medium sized (ipads - pic is from iPad mini) screens the white text gets in front of the light purple mountains making it a little hard to read. If we are ok with this then can ignore but just wanted to bring it up.

@Shivermist is this ok? How should this be handeled?

Could the text be resized to be as much over the mountains as possible (excluding the apply button cause the contrast is high enough)? Worst case I could extend the sky for medium screens so we don't have to worry about text resizing.

@Krish120003
Copy link
Member

image sometimes on medium sized (ipads - pic is from iPad mini) screens the white text gets in front of the light purple mountains making it a little hard to read. If we are ok with this then can ignore but just wanted to bring it up.

@Shivermist is this ok? How should this be handeled?

Could the text be resized to be as much over the mountains as possible (excluding the apply button cause the contrast is high enough)? Worst case I could extend the sky for medium screens so we don't have to worry about text resizing.

Do you think this should be a seperate issue or it must be fixed in this PR

@kabirsg
Copy link
Contributor

kabirsg commented Oct 3, 2024

image
Also just double checking @Shivermist is it ok for on xs screens the 'XI' to be on a separate line. I would think so but would just like to double check

@Shivermist
Copy link

image sometimes on medium sized (ipads - pic is from iPad mini) screens the white text gets in front of the light purple mountains making it a little hard to read. If we are ok with this then can ignore but just wanted to bring it up.

@Shivermist is this ok? How should this be handeled?

Could the text be resized to be as much over the mountains as possible (excluding the apply button cause the contrast is high enough)? Worst case I could extend the sky for medium screens so we don't have to worry about text resizing.

Do you think this should be a seperate issue or it must be fixed in this PR

For readability sake I think it should be fixed in this PR.
@selina-kim would you be able to adjust/resize the text to stay above the mountains as much as possible? If it's giving you too many issues, ping me and I'll send an extended version of the sky so overlapping is completely avoided (if that's a better solution).

@Shivermist
Copy link

image Also just double checking @Shivermist is it ok for on xs screens the 'XI' to be on a separate line. I would think so but would just like to double check

Yeah it should be ok!

@kabirsg
Copy link
Contributor

kabirsg commented Oct 3, 2024

image Also just double checking @Shivermist is it ok for on xs screens the 'XI' to be on a separate line. I would think so but would just like to double check

Yeah it should be ok!

cool ty

@kabirsg
Copy link
Contributor

kabirsg commented Oct 3, 2024

image
Also kinda nitpicky but can this star get moved? This was just on my laptop btw. It doesn't seem to occur on other devices when I look at it though so not sure how fixable it is going to be.

@selina-kim selina-kim dismissed stale reviews from demanr and Krish120003 via 10c4c1b October 3, 2024 05:16
@selina-kim
Copy link
Contributor Author

selina-kim commented Oct 3, 2024

image Also kinda nitpicky but can this star get moved? This was just on my laptop btw. It doesn't seem to occur on other devices when I look at it though so not sure how fixable it is going to be.

addressed it by adding more y-padding for 1280 > screens. is this okay?
there still might be instances of the star overlapping with text on other bigger screen sizes but for now there should be no issue for laptop screen size

@kabirsg
Copy link
Contributor

kabirsg commented Oct 3, 2024

image Also kinda nitpicky but can this star get moved? This was just on my laptop btw. It doesn't seem to occur on other devices when I look at it though so not sure how fixable it is going to be.

addressed it by adding more y-padding for 1280 > screens. is this okay? there still might be instances of the star overlapping with text on other bigger screen sizes but for now there should be no issue for laptop screen size

looks great! Thanks Selina!

kabirsg
kabirsg previously approved these changes Oct 3, 2024
Copy link
Contributor

@kabirsg kabirsg left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for all the work guys!

@kabirsg kabirsg self-requested a review October 4, 2024 03:20
@kabirsg kabirsg dismissed their stale review October 4, 2024 03:21

There are a couple of changes that still need to be made

@kabirsg
Copy link
Contributor

kabirsg commented Oct 4, 2024

image
Idk how we missed this but the date is last year's right now.

Copy link
Contributor

@kabirsg kabirsg left a comment

Choose a reason for hiding this comment

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

Everything looks good to me!

@kabirsg kabirsg requested review from Krish120003 and demanr October 4, 2024 14:14
@Krish120003 Krish120003 merged commit 65f9559 into main Oct 4, 2024
6 checks passed
@Krish120003 Krish120003 deleted the hero branch October 4, 2024 14:35
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.

Create Hero
5 participants