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

Initial HomePage styling #1

Merged
merged 59 commits into from Feb 23, 2022
Merged

Initial HomePage styling #1

merged 59 commits into from Feb 23, 2022

Conversation

Tijani-Dia
Copy link
Collaborator

@Tijani-Dia Tijani-Dia commented Jan 31, 2022

Style all hardcoded components in the HomePage

Copy link
Collaborator

@jhancock532 jhancock532 left a comment

Choose a reason for hiding this comment

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

This is coming together well, some notes on SCSS best practices and thoughts on the next.js components, Link and Image. Using pixel values instead of rem for margins and padding would be nice, but this is a personal preference - Tailwind uses style rules that are fractions and multiples of rem, so we could follow that convention here as well.

components/BottomCTA.tsx Outdated Show resolved Hide resolved
components/BottomCTA.tsx Outdated Show resolved Hide resolved
components/BottomCTA.tsx Outdated Show resolved Hide resolved
components/TopCTA.tsx Outdated Show resolved Hide resolved
components/TopCTA.tsx Outdated Show resolved Hide resolved
styles/_variables.scss Outdated Show resolved Hide resolved
styles/_variables.scss Outdated Show resolved Hide resolved
styles/_variables.scss Outdated Show resolved Hide resolved
styles/globals.scss Outdated Show resolved Hide resolved
pages/index.tsx Show resolved Hide resolved
Co-authored-by: James Hancock <jhancock532@gmail.com>
@thibaudcolas
Copy link
Member

thibaudcolas commented Feb 3, 2022

@Tijani-Dia sorry, I should have shared screenshots right away 🤦 here we are.

I believe this should be round on the ends:

Screenshot 2022-02-03 at 16 09 30

And here is the logo overlapping:

Screenshot 2022-02-03 at 16 09 20

@Tijani-Dia
Copy link
Collaborator Author

@thibaudcolas I've addressed the styling issues mentioned in the previous comment.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Looking good! I’ve suggested one additional change based on my previous review. Additionally I really think the logo should have been done with inline SVG so we can change its color based on the theme more easily – but since it’s done already let’s wait for dark mode to be added before we change that.

styles/_mixins.scss Outdated Show resolved Hide resolved
Co-authored-by: Thibaud Colas <thibaudcolas@gmail.com>
@Tijani-Dia
Copy link
Collaborator Author

Thanks @thibaudcolas. I've done the logo with inline SVG in the following PR.

@thibaudcolas
Copy link
Member

Ah nice :)

@Tijani-Dia Tijani-Dia merged commit 7df6fd3 into main Feb 23, 2022
@Tijani-Dia Tijani-Dia deleted the homepage branch February 23, 2022 10:40
@Tijani-Dia Tijani-Dia mentioned this pull request Feb 28, 2022
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.

None yet

3 participants