Skip to content

Event popup #11

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

Open
JCSnap wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Event popup #11

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac22c7c
Added EventPopup under components to display events with a popup window
JCSnap Feb 11, 2023
e80515e
removed accidental changed made to Navbar.tsx
JCSnap Feb 11, 2023
35abd40
Update EventPopup.tsx
JCSnap Feb 11, 2023
e956010
Edited based on comments
JCSnap Feb 16, 2023
725b5ea
Merge branch 'add-event-popup' of https://github.com/rhdevs/rh-supera…
JCSnap Feb 16, 2023
1f2053b
Merge branch 'main' into add-event-popup
JCSnap Feb 16, 2023
a25d825
1. Added border to Popup container
JCSnap Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions components/sample-rhdevs-website/EventPopup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import React from 'react'

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you try removing this line and see if it works?

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

old habits die hard; there is now no need to write this default import on every page in Next.js now (unless you're explicitely using the React object)

Suggested change
import React from 'react'

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can consider removing this line

import { useState } from 'react'
import styled from 'styled-components'
import SignUpButton from './SignUpButton'

const EventPopupContainer = styled.div`
position: absolute;
background-color: white;
padding: 20px;
margin-top: 100px;
margin-left: 20%;
margin-right: 20%;

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these can be all expressed in one line i.e. margin: .... Go and find out how to use CSS shorthands :)

`

const CloseButton = styled.button`
border-radius: 50%;
display: flex;
margin-left: auto;
background-color: #595959;
color: white;
font-weight: 600;
&:hover {
cursor: pointer;
}
`

const PopupImage = styled.img`

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use Next.js Image component instead, as wen jun has mentioned as well

display: block;
border-radius: 10px;
max-height: 70%;
max-width: 70%;
margin: 0 auto;
padding-top: 30px;
padding-bottom: 50px;

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same, use CSS shorthand

`

const PopupTitle = styled.h1`
display: flex;
font-size: 24px;
font-weight: 600;
color: black;
`
const PopupContent = styled.p`
white-space: pre-line;
`

type Props = {
image: any

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid the use of the type any here. Judging from the use, it seemed to be a string

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
image: any
image: string

yeap, since it's a URL, it's a string. Try to avoid using any

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Avoid using the any type in typescript

title: string
content: string
isActive: boolean
}

const defaultProps = {
image:
'https://images.unsplash.com/photo-1481349518771-20055b2a7b24?ixlib=rb-4.0.3&ixid=MnwxMjA3fDB8MHxzZWFyY2h8NHx8cmFuZG9tfGVufDB8fDB8fA%3D%3D&w=1000&q=80',
title: 'Valentines Day Event',
content:
"Still thinking of what to get your friends or that special someone for VALENTINES DAY? RVC SP got you!!! With a wide assortment of gifts from fresh flowers and preserved flowers , there's bound to be something for everyone! As all stocks are limited, fill up the pre order form HERE to get FIRST DIBS on your items! P.S. Due to unforeseen circumstances, our crochet flowers will not be sold anymore but fret not! The other options are still available so faster PREORDER NOWWWWAll profits will be channelled to support the welfare of our beneficiaries so DONT WAIT ANYMORE!!! Show your love to your friends/ boo and do a good deed today!",
isActive: true,
}

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rmb to remove these ya


function EventPopup(props: Props) {
const [isActive, setIsActive] = useState(true)
const closePopup = () => {
setIsActive(false)
}

return (
<div>
{isActive && (
<EventPopupContainer>
<CloseButton onClick={closePopup}>x</CloseButton>
<PopupImage src={defaultProps.image} alt="Event Image" />

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Consider removing the use of the img tag and use the <Image /> component from Next/Image instead.

Documentation
Reference Link

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeap

<PopupTitle>{defaultProps.title}</PopupTitle>
<PopupContent>{defaultProps.content}</PopupContent>

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should it be

Suggested change
<PopupImage src={defaultProps.image} alt="Event Image" />
<PopupTitle>{defaultProps.title}</PopupTitle>
<PopupContent>{defaultProps.content}</PopupContent>
<PopupImage src={props.image} alt="Event Image" />
<PopupTitle>{props.title}</PopupTitle>
<PopupContent>{props.content}</PopupContent>

@JCSnap JCSnap Feb 11, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yea I'm just using this as a placeholder for testing haha since I think they haven't confirmed the details of how the information will be passed

<SignUpButton />
</EventPopupContainer>
)}
</div>
)
}

export default EventPopup
24 changes: 24 additions & 0 deletions components/sample-rhdevs-website/SignUpButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import React from 'react'

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you try removing this line and see if it works?

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
import React from 'react'

import styled from 'styled-components'

const SignUpButtonContainer = styled.button`

@woowenjun99 woowenjun99 Feb 11, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps this component can be better renamed as a button as it seems to fulfil the functions of a button?

Suggested change
const SignUpButtonContainer = styled.button`
const SignUpButton = styled.button`

@JCSnap JCSnap Feb 11, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hmm it conflicts with the function name if I name it as SignUpButton, is there any way around this?

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const SignUpButtonContainer = styled.button`
const StyledButton = styled.button`

Name it like this instead. Also, there's no need to make an exclusive button just for signups. Instead make a generic button component that can be customised (i.e. take in params that set its display text, onClick function etc.)

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image

mb for not mentioning earlier during the meeting, but i've placed some codes in the sample-rhdevs-websites subfolders, which contains codes that might be useful, so do refer to them if you're unsure of how to make for e.g. a button, or u can just copy them if they work!

display: flex;
background-color: #d9d9d9;
color: black;
font-weight: 600;
padding: 10px;
border-width: 0px;
margin-top: 50px;
margin-left: auto;

@marcus-ong-qy marcus-ong-qy Feb 12, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shorthand

&:hover {
cursor: pointer;
border: 1px solid black;
background-color: white;
}
`

function SignUpButton() {
return <SignUpButtonContainer>Sign Up Here!</SignUpButtonContainer>
}

export default SignUpButton
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@
"axios": "^1.2.6",
"eslint": "8.33.0",
"eslint-config-next": "13.1.6",
"next": "13.1.6",
"react": "18.2.0",
"react-dom": "18.2.0",
"next": "^13.1.6",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-hook-form": "^7.43.0",
"sharp": "^0.31.3",
"styled-components": "^5.3.6",
Expand Down
Loading