-
Notifications
You must be signed in to change notification settings - Fork 1
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
fixes #110 : [V2] Cohort Application Page #117
Conversation
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 add a new state submitted:boolean
and after handleSubmit, disable the button so that people dont spam it. Maybe we can render, already submitted and disable inputs?
<input type="submit" value="Submit" /> | ||
</form> | ||
</section> | ||
</> |
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.
We dont need <></>
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.
This. SInce there is only one child section
we dont need <></>
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.
My mistake. this is needed now
package.json
Outdated
"react": "^18.0.0", | ||
"react-dom": "^18.0.0", | ||
"react-icons": "^4.12.0", | ||
"react-select": "^5.8.0", |
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.
we already have material-ui so can we use those components? https://mui.com/material-ui/react-select?
import Select from 'react-select'; | ||
|
||
const Application = ( | ||
{name}: {name: string} |
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.
we might need one more prop isRegOpen
. We will not be accepting applications for all cohorts simultaneously
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 change to cohortName
, as name is misleading
required | ||
/> | ||
<p>Location*</p> | ||
<input |
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.
The inputs can be made into a single component and looped over
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.
Also check if material-ui has input component, it will look prettier
import Select from 'react-select'; | ||
|
||
const Application = ( | ||
{name}: {name: string} |
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 change to cohortName
, as name is misleading
const [selectedSkills, setSelectedSkills] = useState([]); | ||
const [selectedBooks, setSelectedBooks] = useState([]); |
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.
We can use formData.Skills
and formData.books
?
formData.skills = selectedSkills; | ||
formData.books = selectedBooks; |
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.
We can set this in handleChange itself
@@ -0,0 +1,194 @@ | |||
import React, { useState, type FormEvent } from "react"; | |||
import axios from "axios"; | |||
import { Alert, Autocomplete, Input, TextField } from "@mui/material"; |
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.
Nice work reusing MUI
<input type="submit" value="Submit" /> | ||
</form> | ||
</section> | ||
</> |
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.
This. SInce there is only one child section
we dont need <></>
]; | ||
|
||
const Application = ( | ||
{ cohortName }: { cohortName: string } |
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 add a boolean isRegOpen
so that we open applications only for some cohorts?
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.
Also pls check with @rajarshimaitra which cohort applications should be open when we merge this PR
enrolled: false, | ||
role: cohortName | ||
}); | ||
const [isRegOpen, setIsRegOpen] = useState(regOpen); |
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.
We dont need this state. We can directly use regOpen
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.
makes sense!
onChange={(e, value) => { | ||
setFormData(prevState => ({ | ||
...prevState, | ||
skills: value |
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.
Type of value
is string[]
. This doesnt match the type of skills. Can you fix the type issue?
onChange={(e, value) => { | ||
setFormData(prevState => ({ | ||
...prevState, | ||
books: value |
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.
Type issue just as skills
<input type="submit" value="Submit" /> | ||
</form> | ||
</section> | ||
</> |
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.
My mistake. this is needed now
Also we need to add some validations so that user enters all the fields as right now we allow submission of empty fields |
We will start with the BPD cohort. |
Fixed the type issues, and also changed the order of the questions. here is the new order. @setusaurabh does this look alright to you? also added validation for the required fields. |
@emjshrx lets hold on to this until we finalize the date for BPD. Then we can merge this with only the BPD application open and everything else closed. |
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.
ACK
Created a basic ui for applying to the cohorts. we can change the design based on the revamped design