-
Notifications
You must be signed in to change notification settings - Fork 317
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
[Frontend] Join US Enhancements #790 #864
[Frontend] Join US Enhancements #790 #864
Conversation
Thank you and congrats🎉 for opening this pull request!💖 Please make sure you have followed our Contributing Guidelines.🙌 We will test out your code and reply in a bit with some pointers and requests.💿 There may be some errors, but don't worry! We'll work through them with you! 👍🎉😄 If you haven't filled the template out, Please do so!🚀 |
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.
@SwayamRana808 The changes looks good to me, please incorporate the requested cleanups and it should be good to be merged :)
</div> | ||
</div> | ||
|
||
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.
[neat] please make sure the formatting is consistent, i see few extra space here which is un-necessary
@@ -5,10 +5,14 @@ import Grid from "@material-ui/core/Grid" | |||
import { Card } from './Card/index.js' | |||
import style from './joinus.module.scss' | |||
import Loader from "../../../../components/util/Loader"; | |||
// import { useState } from "react"; |
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.
[neat] please remove these commented code
|
||
const HandleDeleteSuccess = (id) => { | ||
setJoinUsData(prevData => prevData.filter(item => item._id !== id)); | ||
console.log(id); |
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.
[neat] can we please get rid of this console log statement?
@SwayamRana808 one more thing i forgot to point above - There should be a loader while deletion is going on, (take reference from resource deletion page). This will make user aware of the operation going on |
I have made the changes please review it . Recording.2024-05-12.194534.mp4 |
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 👍🏻
Congrats on merging your first pull request! 🙌 🎉 ⚡️ Now that you've merged your first pull request, you're the perfect person to help someone else out with this challenging first step.😄🙌 |
thank you , this was my first open source contribution ❤️ |
Way more to come, all the best @SwayamRana808 :) |
Issue that this pull request solves
Closes: #790
Proposed changes
under admin join us page
Brief description of what is fixed or changed
Types of changes
1.removed url button code.
2. added delete api from backend to delete button from frontend
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that applyScreenshots
Please attach the screenshots of the changes made in case of change in user interface
Other information
Any other information that is important to this pull request