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

Add FAQ page #73

Open
wants to merge 4 commits into
base: pr-review-workshop
Choose a base branch
from
Open

Add FAQ page #73

wants to merge 4 commits into from

Conversation

chromium-52
Copy link
Member

@chromium-52 chromium-52 commented Oct 29, 2023

ℹ️ Issue

Closes

📝 Description

Briefly list the changes made to the code:

  1. Added a page component called FAQ to show a list of commonly asked questions and their answers to those interested in applying to C4C and current members that matches the given Figma designs.
  2. Added an Accordion component that toggles the visibility of the corresponding question's answer for better user experience.

✔️ Verification

What steps did you take to verify your changes work? These should be clear enough for someone to be able to clone the branch and follow the steps themselves.

Checked that the page loads correctly with the given hard-coded questions shown.

Provide screenshots of any new components, styling changes, or pages.

image

🏕️ (Optional) Future Work / Notes

Did you notice anything ugly during the course of this ticket? Any bugs, design challenges, or unexpected behavior? Write it down so we can clean it up in a future ticket!

},
});

const Accordion: React.FC<AccordionProps> = (props: any) => {

Choose a reason for hiding this comment

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

🔧 Props should be typed

Choose a reason for hiding this comment

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

Suggested change
const Accordion: React.FC<AccordionProps> = (props: any) => {
const Accordion: React.FC<AccordionProps> = ({ sections }: AccordionProps) => {

@@ -43,6 +44,7 @@ const App: React.FC = () => {
<Route path="/apply/brand-designer" Component={ApplyBrandDesigner} />
<Route path="/projects" Component={Projects} />
<Route path="/people" Component={People} />
{/* <Route path="/faq" Component={FAQ} /> */}

Choose a reason for hiding this comment

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

Why is this commented out?

@@ -43,6 +44,7 @@ const App: React.FC = () => {
<Route path="/apply/brand-designer" Component={ApplyBrandDesigner} />
<Route path="/projects" Component={Projects} />
<Route path="/people" Component={People} />
{/* <Route path="/faq" Component={FAQ} /> */}

Choose a reason for hiding this comment

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

🔧 Comment can be removed

},
{
title: 'How does the application process work?',
// TODO figure out how to link to the apply page from here
Copy link

@airman416 airman416 Oct 29, 2023

Choose a reason for hiding this comment

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

🔧 Remove comment
❓ Has the TODO been completed or pending?

import { makeStyles } from '@material-ui/core/styles';

interface AccordionProps {
sections: any[];
Copy link

@leahzeisner leahzeisner Oct 29, 2023

Choose a reason for hiding this comment

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

It might be beneficial to create a section type to be used here in place of type any[]

Choose a reason for hiding this comment

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

+1 Possible type might be something like

interface Section {
  title: string,
body: React.Element
}

<AccordionSummary
expandIcon={<ExpandMoreIcon />}
onClick={() => {
console.log('onClick called');

Choose a reason for hiding this comment

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

🔧 can get rid of this debug print

title: 'What does C4C stand for?',
body: 'C4C stands for Code4Community!',
},
{

Choose a reason for hiding this comment

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

Maybe abstract the title bodies in this function

Copy link

@SurabhiKeesara SurabhiKeesara left a comment

Choose a reason for hiding this comment

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

Looks good, but we have a few questions

@@ -43,6 +44,7 @@ const App: React.FC = () => {
<Route path="/apply/brand-designer" Component={ApplyBrandDesigner} />
<Route path="/projects" Component={Projects} />
<Route path="/people" Component={People} />
{/* <Route path="/faq" Component={FAQ} /> */}

Choose a reason for hiding this comment

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

🔧 Is this meant to be commented out?

Choose a reason for hiding this comment

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

+1 If so, please leave a comment explaining why and when it should be uncommented.

export default function FAQ() {
// this is the list of FAQs
const frequently_asked_questions = [
{

Choose a reason for hiding this comment

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

🔧 Can we move these {title, body} constants out of this file? Maybe to a different file?

{
title: 'How does the application process work?',
// TODO figure out how to link to the apply page from here
body: "Visit the apply page and submit your application. We'll reach out to schedule an interview with 1-2 members of our team.",

Choose a reason for hiding this comment

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

🔧 It looks like line 46 inserts a link, that can maybe be implemented here.

Copy link

@leahzeisner leahzeisner left a comment

Choose a reason for hiding this comment

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

Looks good overall, left a few small comments

<AccordionSummary
expandIcon={<ExpandMoreIcon />}
onClick={() => {
console.log('onClick called');

Choose a reason for hiding this comment

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

🔧 Log can be removed

@@ -0,0 +1,66 @@
// Mostly copied from src/app/components/Accordion.tsx

Choose a reason for hiding this comment

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

❓ If it's mostly copied, why not just modify that Accordion to support this use case? What are the major differences?

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.

7 participants