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

Feature/hrithik/congratulationsPopupWindow #24

Merged
merged 17 commits into from
Apr 2, 2024

Conversation

hpai1
Copy link
Contributor

@hpai1 hpai1 commented Feb 28, 2024

Tracking Info

Resolves #16

Changes

Imported some new packages, so be sure to run npm install.

I added a pop-up window component that can be called once the user submits the form.

The button can be in the original page and using the onClick method, it can open the component. The component opens as a popup and dims the background.

This is an example call that would be nested in the original page:

<CongratulationsPopupWindow buttonText="Congratulations Window" />, where buttonText is the text that should be in the button. The button automatically takes the style of the Button.tsx component.

When the button is clicked on the popup or the close button is clicked, it redirects the user to the home page (https://ccidc.org/).

I also modified the behavior of the Button component to where the onClick is not specified by default, so you have to use "undefined" if the button should not have an onClick: <Button onClick={undefined}></Button>.

Testing

To test the changes worked, I ran it on my machine by adding a custom button to the Candidates Tab called "Congratulations Window", which opens the popup when clicked.

Confirmation of Change

Screenshot 2024-02-27 at 11 03 42 PM

After the button is clicked:
Screenshot 2024-02-27 at 11 03 58 PM

After either the close button or the "Click here.." button is clicked:
Screenshot 2024-02-27 at 10 56 45 PM

@hpai1 hpai1 requested a review from Miyuki-L as a code owner February 28, 2024 07:06
@Miyuki-L Miyuki-L requested review from mohakvni and emmaz12 March 1, 2024 03:46
frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
buttonText: string; // Define prop for button text
}
const CongratulationsPopupWindow: React.FC<CongratulationsPopupWindowProps> = ({ buttonText }) => {
const [isOpen, setIsOpen] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

I think the state of the popup should go outside the pop up this way we can manage the state of the popup from outside. We many not always be triggering the popup using a button. Or at least a way to set the state of the popup from outside the component

Either with state logic outside for example the twobuttonpopup here

Or with a way to set the logic from outside like how Accordian is used in FAQ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example on Candidates.tsx

</div>
<div className={styles.spacer} style={{ height: "2px" }}></div> {/* Added spacer */}
<div className={styles.closeButton}>
<Button onClick={closePopupAndReturn} children="Click here to return to Main Page" />
Copy link
Member

Choose a reason for hiding this comment

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

Chidren can be passed through passing the content through opening and closing tags. Children is a keyworkd and react knows what to do. So it'll be <Button> Click here to return to Main Page </Button>

Also the padding on the button looks a bit thin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to change it in the Button.module.css file since this is a button component.

frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
frontend/package.json Show resolved Hide resolved
Copy link
Member

@Miyuki-L Miyuki-L left a comment

Choose a reason for hiding this comment

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

Almost there. Just a few tiny things to clean up. Great work though. Make sure to also pull from main.


const handleKeyPress = (event: React.KeyboardEvent<HTMLDivElement>) => {
if (event.key === "Enter" || event.key === " ") {
onClose();
Copy link
Member

Choose a reason for hiding this comment

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

Are we purposely having this case not navigate to ccidc.org?


return (
<div>
<Button onClick={onClose}>{buttonText}</Button>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this button should belong in congratulations popup. This results in when ever we use the popup the will show up but we don't want that.

frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
cursor: pointer;
}

.closeButton:hover {
Copy link
Member

Choose a reason for hiding this comment

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

This is a repeat on the cursor style already applied by button.module.css

<img src={closeIcon} alt="Close Popup" />
</button>
<img className={styles.checkMark} src={checkMark} alt="failed to load" />
<div className={styles.mainText}> Congratulations! </div>
Copy link
Member

Choose a reason for hiding this comment

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

can we change this div and the div bellow to <p> they make more semantic sense. though it has some styling impacts so either remove the stylings on p or adjust the existing stylings

frontend/src/components/CongratulationsPopupWindow.tsx Outdated Show resolved Hide resolved
frontend/src/components/Button.tsx Outdated Show resolved Hide resolved
</div>
);
};
export default CongratulationsPopupWindow;
Copy link
Member

Choose a reason for hiding this comment

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

can we just export function this and add the export to the index.ts file in this folder? Just to keep it consistent with the rest of the code.

}

export default Candidates;
Copy link
Member

Choose a reason for hiding this comment

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

can we just export function this. Just to keep it consistent with rest of the code

Copy link
Contributor

@emmaz12 emmaz12 left a comment

Choose a reason for hiding this comment

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

Other than the comments Helen suggested, I don't see any issues or inconsistencies.
Great job!

@Miyuki-L Miyuki-L merged commit ff3b9a8 into main Apr 2, 2024
2 checks passed
@Miyuki-L Miyuki-L deleted the feature/Hrithik/CongratulationsPopupWindow branch April 2, 2024 07:19
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.

Modals
4 participants