-
Notifications
You must be signed in to change notification settings - Fork 53
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
base: pr-review-workshop
Are you sure you want to change the base?
Add FAQ page #73
Conversation
}, | ||
}); | ||
|
||
const Accordion: React.FC<AccordionProps> = (props: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Props should be typed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} /> */} |
There was a problem hiding this comment.
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} /> */} |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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[]; |
There was a problem hiding this comment.
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[]
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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!', | ||
}, | ||
{ |
There was a problem hiding this comment.
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
There was a problem hiding this 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} /> */} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 = [ | ||
{ |
There was a problem hiding this comment.
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.", |
There was a problem hiding this comment.
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.
There was a problem hiding this 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'); |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
ℹ️ Issue
Closes
📝 Description
Briefly list the changes made to the code:
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.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.
🏕️ (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!