Adding execom member. ending tenure of execom #101
Conversation
sannidhi-s-shetty
requested review from
MayurSChittaragi and
Prajwalprakash3722
Prajwalprakash3722
left a comment
Prajwalprakash3722
left a comment
There was a problem hiding this comment.
Overall the changes look good,lot of code needs to be reorganised and cleaned
But the proof of concept is now clear.
|
maybe try to comment out the unused variables next time, at least it will pass the test |
|
can we redesign the layout like this?, makes easy to identify the buttons. |
Prajwalprakash3722
left a comment
Prajwalprakash3722
left a comment
There was a problem hiding this comment.
Overall it looks neat and properly organised, I will pull the branch test it out and report if any issues.
Prajwalprakash3722
left a comment
Prajwalprakash3722
left a comment
There was a problem hiding this comment.
Overall it looks neat with minor changes
| const expectedValue = 'my life my choice'; | ||
|
|
||
| if (inputValue === expectedValue) { | ||
| console.log("match"); |
There was a problem hiding this comment.
instead of consoling the match, can we have a SnackBar that will show the user result
There was a problem hiding this comment.
if there is a match, then deleteExecom() gets called, for that there is a Snackbar telling ended tenure or not. Is there a need for another one?
There was a problem hiding this comment.
if the user input is not correct, how can we inform them? any other idea
There was a problem hiding this comment.
If strings do not match, below the text field a string "strings do not match" gets displayed for 2 seconds.
And if string matches, a Snackbar telling whether the deletion was successful or not gets displayed.
| <br/> | ||
| Please type <i>my life my choice</i> to confirm. | ||
| </DialogContentText> | ||
| <TextField |
There was a problem hiding this comment.
can we mark the border of the input incase if the user does not enter the required string, when they enter the required string turn the input border as green (Just an Idea)
src/components/AddExecomDialog.js
Outdated
|
|
||
|
|
||
| useEffect(() => { | ||
| if (props.data !== undefined) { |
There was a problem hiding this comment.
Question: Are we handling the edit action? if so where and how?
There was a problem hiding this comment.
not as of now.....just kept the code from events.
|
@sannidhi-s-shetty any update on this? |
Prajwalprakash3722
left a comment
Prajwalprakash3722
left a comment
There was a problem hiding this comment.
I have couple of doubts,
-
If we are fetching the members in
AlumniAccordionspage why are we passingmembersas props? -
How one can add/edit the execom? where did the editing option go?
There was a problem hiding this comment.
can you elaborate more on this file?
| </Paper> */} | ||
|
|
||
| <SocietyExec sid = {ecats.aps}/> | ||
| <AlumniAccordions members={alumni.aps} sid={ecats.aps} color="rgb(110 110 193)" /> |
There was a problem hiding this comment.
why are we passing the member as props where it is being fetched in the AlumniAccordions Component?
Form to take details of execom member. And a delete execom based on the sid feature added.