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

Jsimj001/home page secondary text #25

Merged
merged 12 commits into from
Jan 31, 2025
Merged

Conversation

JSimjee
Copy link
Collaborator

@JSimjee JSimjee commented Jan 16, 2025

The font type is still somewhat different than what the figma shows, and I'm not sure whether we are supposed to implement the white font color, but I could not get that working anyway.
I made a new fontFamily in the tailwind.config.ts file as well.

Comment on lines 14 to 15
"white-255": "FFFFFF",
"black-255": "000000",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"white-255": "FFFFFF",
"black-255": "000000",

remove this, there are already classes on tailwind for this

Comment on lines 19 to 24
fontFamily: {
'cvdsa-Montserrat-sans': [
'Montserrat',
'sans-serif',
],
},
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this too, montserrat is the default font of the repository. if you can't see it, then that means you need to make sure your dev branch is up to date and pull from dev

Suggested change
fontFamily: {
'cvdsa-Montserrat-sans': [
'Montserrat',
'sans-serif',
],
},

Comment on lines 16 to 19

// Comments by Jameel:
// Where I found these sources:
// Fonts: https://tailwindcss.com/docs/font-family
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comments

Suggested change
// Comments by Jameel:
// Where I found these sources:
// Fonts: https://tailwindcss.com/docs/font-family

Comment on lines 3 to 9
<div className="
text-5x1/48.76
font-cvdsa-Montserrat-sans
font-[500]
flex h-screen w-screen items-center text-center mx-auto justify-center
text-[4vw]
text-cvdsa-white-255">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="
text-5x1/48.76
font-cvdsa-Montserrat-sans
font-[500]
flex h-screen w-screen items-center text-center mx-auto justify-center
text-[4vw]
text-cvdsa-white-255">
<div className="
flex h-screen w-screen items-center text-center mx-auto justify-center
text-[4vw] ">

just a rough approximation of the font is fine, it doesn't need to be crazy. if text-5xl looks close enough to the figma, you can use that. montserrat is the default font, remove those styles. the color white is already added within tailwind as text-white

@stanleylew5
Copy link
Contributor

Link your issue and assign the pr to yourself

@stanleylew5
Copy link
Contributor

Make sure you add the respective labels too

@JSimjee JSimjee self-assigned this Jan 17, 2025
@JSimjee
Copy link
Collaborator Author

JSimjee commented Jan 23, 2025

I have applied the requested changes from this pull request comment chain. Please review them and let me know of any required changes, so that I can work on the next issue accordingly.

Copy link
Contributor

@stanleylew5 stanleylew5 left a comment

Choose a reason for hiding this comment

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

image
Resolve your merge conflict locally

@JSimjee JSimjee dismissed stanleylew5’s stale review January 25, 2025 10:52

the requested change has been resolved locally, the merge conflict was resolved

@@ -0,0 +1,9 @@
const HomePage = () => {
return (
<div className="mx-auto flex h-screen w-screen items-center justify-center text-center text-[4vw] text-black">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div className="mx-auto flex h-screen w-screen items-center justify-center text-center text-[4vw] text-black">
<div className="mx-auto flex w-screen items-center justify-center text-center text-[4vw] text-black">

style the text a bit more so the distance to the main text and size relative to the main text match the figma; looks good otherwise

Copy link
Contributor

@minhhdtran minhhdtran left a comment

Choose a reason for hiding this comment

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

check comments

Copy link
Contributor

@stanleylew5 stanleylew5 left a comment

Choose a reason for hiding this comment

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

lgtm!

@stanleylew5 stanleylew5 merged commit d9f27be into dev Jan 31, 2025
5 checks passed
@stanleylew5 stanleylew5 deleted the jsimj001/home-page-secondary-text branch January 31, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants