Skip to content
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

Remove users modal Implemented #934

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

Aady7
Copy link
Contributor

@Aady7 Aady7 commented Oct 30, 2023

Implemented the modal to remove users of a org using

Description

Implemented modal to remove users from a organization by entering the user handle and called removeOrgUser function on submit of the modal.

Related Issue

#890 solved

Motivation and Context

How Has This Been Tested?

Tested locally on my system.

Screenshots or GIF (In case of UI changes):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Aady7
Copy link
Contributor Author

Aady7 commented Oct 31, 2023

@shivareddy6 I have also implemented the remove use functionality for the orgs ,

Recording.2023-10-31.225842_compressed.mp4

@shivareddy6
Copy link
Collaborator

same here, please resolve the conflicts

@Aady7
Copy link
Contributor Author

Aady7 commented Nov 21, 2023

@shivareddy6 I have resolved this conflict too..Pls merge it after merging the add modal pr.

@@ -15,6 +21,28 @@ const useStyles = makeStyles(theme => ({

function Users() {
const classes = useStyles();
const [modalOpenAdmin, setModalOpenAdmin] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not utilising any of these states in this component expect for sending them as props right, so i suggest you redefine all the states related modal in OrgUsers component itself.
This will:

  1. Improve locality of the states and is easy to track their functionality
  2. Avoid un-necessary re-renders in this component due to the state change of states related to some other component

const closeContriModal = () => {
setModalOpenContr(false);
};
const handleAddContri = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same goes for these functions

@shivareddy6
Copy link
Collaborator

@shivareddy6 I have also implemented the remove use functionality for the orgs ,

Recording.2023-10-31.225842_compressed.mp4

the UI implementation is good, but can you include the backend details as well
It would be great to see what those buttons are doing. Click those buttons and show the changes that are made in the database due to it.

Please ask if you have any doubts.

@Aady7
Copy link
Contributor Author

Aady7 commented Jan 1, 2024

@shivareddy6 thanks for the review I would work to incorporate the suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants