Event popup by JCSnap · Pull Request #11 · rhdevs/rh-superapp

JCSnap · 2023-02-11T12:18:03Z

Added EventPopup under components to display events with a popup window
Added a placeholder SignUpButton under components for the popup

Added a placeholder SignUpButton under components for the popup

by deleted "import EventPopup...." statement which was used for testing

JCSnap

removed accidental changes

woowenjun99

Hi I have left some general comments for your code

woowenjun99 · 2023-02-11T14:58:34Z

@@ -0,0 +1,84 @@
+import React from 'react'


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

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 · 2023-02-11T14:59:26Z

@@ -0,0 +1,24 @@
+import React from 'react'


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

Suggested change

import React from 'react'

woowenjun99 · 2023-02-11T15:00:20Z

+import React from 'react'
+import styled from 'styled-components'
+
+const SignUpButtonContainer = styled.button`


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`

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

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.)

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!

woowenjun99 · 2023-02-11T15:02:14Z

+`
+
+type Props = {
+  image: any


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

Suggested change

image: any

image: string

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

woowenjun99 · 2023-02-11T15:05:08Z

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


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

Documentation
Reference Link

woowenjun99 · 2023-02-11T15:10:25Z

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


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>

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

woowenjun99 · 2023-02-11T15:11:05Z

+`
+
+type Props = {
+  image: any


Avoid using the any type in typescript

woowenjun99 · 2023-02-11T15:11:33Z

@@ -0,0 +1,84 @@
+import React from 'react'


Can consider removing this line

marcus-ong-qy

Tried to visualise your component, and it seems good so far! But do remember for future PRs do include images of whatever you have done ya (e.g. below)

Also I've left some comment for you, do review them. Good job!

It'll be great if you could document your code as well: guide

marcus-ong-qy · 2023-02-12T15:05:30Z

+  margin-top: 100px;
+  margin-left: 20%;
+  margin-right: 20%;


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

marcus-ong-qy · 2023-02-12T15:06:57Z

+  }
+`
+
+const PopupImage = styled.img`


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

marcus-ong-qy · 2023-02-12T15:07:15Z

+  padding-top: 30px;
+  padding-bottom: 50px;


same, use CSS shorthand

marcus-ong-qy · 2023-02-12T15:08:06Z

+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,
+}


rmb to remove these ya

marcus-ong-qy · 2023-02-12T15:09:22Z

+  margin-top: 50px;
+  margin-left: auto;


1. Removed import react 2. Changed SignUpButton to generic button that takes in onClick function 3. Changed img to using <Image /> from Next 4. Changed img type to String 5. Added documentation for EventPopUp 6. Deleted defaultProps placeholder 7. Changed CSS to use shorthands

… into add-event-popup

2. Made the Popup container more responsive by reducing left and right margins for smaller screens

marcus-ong-qy

much better! just work on writing the documrntation, and a few more comments

marcus-ong-qy · 2023-03-14T15:47:31Z

+/**
+ *
+ * @returns A popup that displays the event details and sign up button
+ *
+ */


ok good! but can follow the format here? https://www.notion.so/How-To-Frontend-Documentation-390a6a02b00d4eb59fd7fa9563025885

marcus-ong-qy · 2023-03-14T15:48:16Z

+    <div>
+      {isActive && (
+        <EventPopupContainer>
+          <CloseButton onClick={closePopup}>x</CloseButton>
+          <PopupImage src={props.image} alt="Event Image" />
+          <PopupTitle>{props.title}</PopupTitle>
+          <PopupContent>{props.content}</PopupContent>
+          <SignUpButton />
+        </EventPopupContainer>
+      )}
+    </div>


Suggested change

<div>

{isActive && (

<EventPopupContainer>

<CloseButton onClick={closePopup}>x</CloseButton>

<PopupImage src={props.image} alt="Event Image" />

<PopupTitle>{props.title}</PopupTitle>

<PopupContent>{props.content}</PopupContent>

<SignUpButton />

</EventPopupContainer>

)}

</div>

<>

{isActive && (

<EventPopupContainer>

<CloseButton onClick={closePopup}>x</CloseButton>

<PopupImage src={props.image} alt="Event Image" />

<PopupTitle>{props.title}</PopupTitle>

<PopupContent>{props.content}</PopupContent>

<SignUpButton />

</EventPopupContainer>

)}

</>

if outermost div is not styled (i.e. dummy div), then can just use a blank tag

Added EventPopup under components to display events with a popup window

ac22c7c

Added a placeholder SignUpButton under components for the popup

JCSnap requested a review from marcus-ong-qy February 11, 2023 12:18

removed accidental changed made to Navbar.tsx

e80515e

by deleted "import EventPopup...." statement which was used for testing

JCSnap commented Feb 11, 2023

View reviewed changes

Update EventPopup.tsx

35abd40

woowenjun99 reviewed Feb 11, 2023

View reviewed changes

marcus-ong-qy suggested changes Feb 12, 2023

View reviewed changes

JCSnap and others added 4 commits February 16, 2023 19:59

Merge branch 'add-event-popup' of https://github.com/rhdevs/rh-superapp…

725b5ea

… into add-event-popup

Merge branch 'main' into add-event-popup

1f2053b

1. Added border to Popup container

a25d825

2. Made the Popup container more responsive by reducing left and right margins for smaller screens

JCSnap requested a review from marcus-ong-qy February 17, 2023 04:13

marcus-ong-qy suggested changes Mar 20, 2023

View reviewed changes

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

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

Uh oh!

Conversation

JCSnap commented Feb 11, 2023

Uh oh!

JCSnap left a comment

Choose a reason for hiding this comment

Uh oh!

woowenjun99 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-ong-qy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcus-ong-qy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants