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

feat(web): add remove member button #841 #888

Merged
merged 11 commits into from
Jul 27, 2022

Conversation

MaurerKrisztian
Copy link
Contributor

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feature

  • What is the current behavior? (You can also link to an open issue here)
    Team Members management #841

  • What is the new behavior (if this is a feature change)?
    Admin can remove invitation or members (if backend enables)
    image

  • Other information:

@vercel
Copy link

vercel bot commented Jul 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
docs ✅ Ready (Inspect) Visit Preview Jul 27, 2022 at 1:42PM (UTC)

Copy link
Contributor

@scopsy scopsy left a comment

Choose a reason for hiding this comment

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

Thank you so much for the contribution @MaurerKrisztian I've attached a design from the Figma project in the comment. Could you please take a look at it?

Comment on lines 45 to 62
async function removeMemberClick(member) {
await removeMember(member._id)
.then((result) => {
showNotification({
message: `Successful member deletion.`,
color: 'green',
});
refetch();

return result;
})
.catch((err) => {
showNotification({
message: err.message,
color: 'red',
});
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
async function removeMemberClick(member) {
await removeMember(member._id)
.then((result) => {
showNotification({
message: `Successful member deletion.`,
color: 'green',
});
refetch();
return result;
})
.catch((err) => {
showNotification({
message: err.message,
color: 'red',
});
});
}
async function removeMemberClick(member) {
try {
await removeMember(member._id)
showNotification({
message: `Successful member deletion.`,
color: 'green',
});
refetch();
} catch (e) {
showNotification({
message: err.message,
color: 'red',
});
}
}

A small suggestion can be refactored to use plain async await like the rest of the codebase. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

Comment on lines 110 to 112
<span onClick={() => removeMemberClick(member)}>
<Tag css={{ cursor: 'pointer' }}>X</Tag>
</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please checkout the design at figma:
https://www.figma.com/file/IpAw7sGWYZ8DHTem0tSM8r/Ready-for-Dev?node-id=28%3A7377

So we can actually have the 3 dots with some space around, and upon clicking we can use mantine's dropdown with a delete button. You can also see Workflow editor for example.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

CleanShot 2022-07-25 at 20 14 42@2x

Here is how it looks from the workflow editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the example, fixed it.
image

Comment on lines 3 to 29
export function ThreeDot({ ...props }) {
const { theme } = useStyles();

const dot = {
height: '7px',
width: '7px',
margin: '2px',
'background-color': theme.colorScheme == 'dark' ? '#525266' : '#525266',
'border-radius': '50%',
display: 'inline-block',
'font-weight': 'bold',
};

const threeDot = {
cursor: 'pointer',
padding: '5px',
margin: '5px',
};

return (
<div style={threeDot}>
<span style={dot}></span>
<span style={dot}></span>
<span style={dot}></span>
</div>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for missed this one, but we actually just have an SVG icon for the three dots so no need to recreate it. It located in apps/web/src/design-system/icons/general/DotsHorizontal.tsx So you can just import it instead of building it with CSS.

🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, sorry I didn't noticed

@scopsy
Copy link
Contributor

scopsy commented Jul 26, 2022

This now looks awesome @MaurerKrisztian, been playing with it right now. A few things noticed:

  • I think we should remove the dots button for the current user in the list (it's strange I can remove myself ;P even tho the API blocks me from doing so)
  • We should show the remove dots dropdown only if the current logged in user role is ADMIN.

Wdyt?

@MaurerKrisztian
Copy link
Contributor Author

MaurerKrisztian commented Jul 26, 2022

This now looks awesome @MaurerKrisztian, been playing with it right now. A few things noticed:

  • I think we should remove the dots button for the current user in the list (it's strange I can remove myself ;P even tho the API blocks me from doing so)
  • We should show the remove dots dropdown only if the current logged in user role is ADMIN.

Wdyt?

@scopsy Good idea. I added that.

Copy link
Contributor

@scopsy scopsy left a comment

Choose a reason for hiding this comment

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

Thank you so much @MaurerKrisztian for your contribution, I've added some unit tests for this aswell with the latest cypress component update. Merging this!

@scopsy scopsy merged commit 1631568 into novuhq:main Jul 27, 2022
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.

None yet

2 participants