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.

Sign up for GitHub

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

Jump to bottom

feat(HeroBanner): add HeroBanner component and add to HomePage #10

Merged
merged 40 commits into from
Jul 17, 2024
Merged

feat(HeroBanner): add HeroBanner component and add to HomePage #10

merged 40 commits into from
Jul 17, 2024

Conversation

Kara-Zor-El
Copy link
Contributor

@Kara-Zor-El Kara-Zor-El commented Jun 13, 2024 •
edited
Loading

Screenshot

image

Description

  • Adds HeroBanner as seen on the main page of Figma design

@Kara-Zor-El Kara-Zor-El added the enhancement New feature or request label Jun 13, 2024
spotandjake
spotandjake reviewed Jun 13, 2024
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

Mostly looks good two nits

components/HeroBanner.tsx Outdated Show resolved Hide resolved
components/HeroBanner.tsx Outdated Show resolved Hide resolved
amberwelby
amberwelby previously approved these changes Jun 14, 2024
View reviewed changes
Copy link
Member

@amberwelby amberwelby left a comment

Choose a reason for hiding this comment

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

One comment about title margins on Discord, but otherwise looks great!

@Kara-Zor-El Kara-Zor-El force-pushed the KaraZorEl-HeroBanner branch from afa5cc9 to 704ed95 Compare July 16, 2024 00:51
spotandjake
spotandjake reviewed Jul 16, 2024
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

Mostly looks really good, a few nits.

components/Button.tsx Show resolved Hide resolved
components/Button.tsx Show resolved Hide resolved
components/Button.tsx Outdated Show resolved Hide resolved
components/Button.tsx Outdated Show resolved Hide resolved
components/HeroBanner.tsx Outdated Show resolved Hide resolved
styles/Button.module.scss Outdated Show resolved Hide resolved
styles/Button.module.scss Show resolved Hide resolved
styles/Button.module.scss Outdated Show resolved Hide resolved
styles/components/HeroBanner.module.scss Outdated Show resolved Hide resolved
tailwind.config.ts Show resolved Hide resolved
@Kara-Zor-El Kara-Zor-El force-pushed the KaraZorEl-HeroBanner branch from 4462511 to 868c63b Compare July 16, 2024 03:01
@Kara-Zor-El Kara-Zor-El force-pushed the KaraZorEl-HeroBanner branch from 868c63b to 856ed65 Compare July 16, 2024 03:05
spotandjake
spotandjake previously approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

Code looks great :)

amberwelby
amberwelby approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@amberwelby amberwelby left a comment

Choose a reason for hiding this comment

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

Awesome work here, looks great to me!

components/Button.tsx Outdated
import Link from 'next/link';
import styles from '../styles/Button.module.scss';

enum ButtonType {
Copy link
Member

Choose a reason for hiding this comment

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

Love this

tailwind.config.ts
@@ -18,6 +18,7 @@ const config: Config = {
'hover-1': '#4db6ac', // TODO: Decide on a color for this
'hover-2': '#89B4FA', // TODO: Decide on a color for this
'event-banner': '#5E18EB', // Event Banner Background
socials: '#7BD4CB', // Social
Copy link
Member

Choose a reason for hiding this comment

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

Is there a functional/semantic reason socials isn't in quotes and the rest are?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a functional/semantic reason socials isn't in quotes and the rest are?

The others have a - in their name which means they have to be in quotes the formatter removes them on this one because they are not needed. We will probably change this name with issue #16

amberwelby
amberwelby previously approved these changes Jul 16, 2024
View reviewed changes
Copy link
Member

@amberwelby amberwelby left a comment

Choose a reason for hiding this comment

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

Sorry for the second review, still think it looks great but had more thoughts

@Kara-Zor-El Kara-Zor-El dismissed stale reviews from amberwelby and spotandjake via faf5ccb July 16, 2024 12:13
spotandjake
spotandjake approved these changes Jul 17, 2024
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

Just two very small changes but otherwise I'm giving approval feel free to merge after that.

components/EventBanner.tsx Outdated

if (bannerInfo.length === 0 || bannerInfo?.hidden) {
if (!bannerInfo || (Array.isArray(bannerInfo) && bannerInfo.length === 0) || (bannerInfo as any)?.hidden) {
Copy link
Member

Choose a reason for hiding this comment

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

The as any is not the way todo this can you add a todo linking to this issue like //TODO(#12): Improve yaml typechecking.

components/EventBanner.tsx Outdated
return null;
}

const eventMarkdown = bannerInfo.text;
const eventMarkdown = (bannerInfo as any).text;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above //TODO(#12): Improve yaml typechecking

@Kara-Zor-El Kara-Zor-El merged commit 4d7b78b into main Jul 17, 2024
1 check passed
@Kara-Zor-El Kara-Zor-El deleted the KaraZorEl-HeroBanner branch July 17, 2024 01:12
@Kara-Zor-El Kara-Zor-El mentioned this pull request Jul 17, 2024
Merged
7 tasks
Kara-Zor-El added a commit that referenced this pull request Aug 7, 2024
@Kara-Zor-El
Feat(HomePage): Create Home Page (#18)
78ce949 
Link to Preview:
https://club-website-c1tjlgepg-karas-projects-e1ba5d81.vercel.app
<img width="1712" alt="image"
src="https://github.com/user-attachments/assets/5ebc1da2-6145-4675-b6b5-a6a18c367b5c">

## Needed Additions
- [X] Hero banner (#10)
- [x] Section Headers (#20)
- [X] About Us Section (Accidentally put on this branch)
- [X] Upcoming Events (with fallbacks) (Will do so in future PR)
- [X] Latest News / Blogs (#23 for initial UI without content fetching)
- [X] Contact Us (#21)
- [x] Footer (#22)

## Additional notes:
- I have not made this work on mobile yet. I figure I'll do so later and
instead make sure it looks good on just desktop for now.
- Latest News / Blogs section doesn't have content fetching
- We will wait for event page in later pr
- Event section not included (and won't be until the events page is
merged due to borrowing components)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amberwelby amberwelby amberwelby left review comments

@spotandjake spotandjake spotandjake approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Kara-Zor-El @spotandjake @amberwelby