-
Notifications
You must be signed in to change notification settings - Fork 77
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 feature to create, view, edit & delete forms #39
Conversation
@abha224 @sidvenu As I had mentioned, work on this PR is left, but you can review this part. Also, there is a slight change in the UI than mentioned in the design, but I don't know why this looks more balanced and prettier. If you want me to shift to the mentioned design, I will but please give this view a thought. 😃 |
src/components/Form.js
Outdated
class Form extends Component { | ||
constructor(props) { | ||
super(props) | ||
} | ||
|
||
componentDidMount() { | ||
this.props.getInfo() | ||
} | ||
|
||
export default class Form extends Component { | ||
render() { | ||
const { userinfo } = this.props | ||
const type = userinfo ? ( userinfo[0] ? userinfo[0].user_type : null ) : 'student' | ||
return ( | ||
<div> | ||
forms | ||
<div className='form'> | ||
<div className='inside'> | ||
{ | ||
type === 'admin'? | ||
<> | ||
<Header>Published Forms</Header> | ||
<PublishedForm /> | ||
<Divider /> | ||
<Header>Unpublished Forms</Header> | ||
<UnpublishedForm /> | ||
</> | ||
: <PublishedForm /> | ||
} | ||
</div> | ||
</div> | ||
) | ||
} | ||
} | ||
|
||
Form.propTypes = { | ||
userinfo: PropTypes.array.isRequired | ||
}; | ||
|
||
const mapStateToProps = state => ({ | ||
formerror: state.form.formerror, | ||
userinfo: state.info.userinfo, | ||
}) | ||
|
||
export default connect( | ||
mapStateToProps, | ||
{ postForm, getInfo } | ||
)(Form) |
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.
Could you explain this component? I'm not able to wrap my head around it.
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 is supposed to render based on condition of which user is logged in.
Do address the above :) |
@bismitaguha I tried looking into the bug you described in our 1:1, I'm not able to track it down just by code review. Will test it out and try to find out the bug soon. |
@bismitaguha did you get any leads on this bug (I didn't)? If not, we can use today's OSP weekly session to debug it if we have time in the end. cc @abha224 |
@bismitaguha I was going through the working of this branch, and seems like theres a problem in user info update. Did you modify the code for that ? |
There is a warning I guess, but this won't be the PR where I make that change, I haven't made any changes to that component in this PR. |
Add feature to add options wherever required
@bismitaguha There are 2 class components 'Form' and 'Forms' in forms.js and dashboard.js. And even the class name for Submissions.js is Form. Pls modify these class names appropriately to avoid duplicates. |
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.
Please make the changes @abha224 requested. Were the bugs you talked about previously fixed?
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.
LGTM. One general suggestion is for you to add more comments wherever it makes sense - but it has been 4 weeks already, let's get this PR merged.
@bismitaguha the buttons on the form are not functional? Also, the forms are not being retained after creation? (pls ignore spelling errors in gif ) |
Weirdly create function in mine is working fine....We can discuss this in Siddharth's office hours today. I guess maybe you are in some other branch or something, just like the problem was previous time!!! @abha224 |
Description
moment.js
for display of datesFixes #19
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
UI
Checklist:
Code/Quality Assurance Only