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

Added delete buttons for hacknight participants #417

Conversation

jacekkalbarczyk
Copy link
Contributor

Story / Bug id:

#364

Description:

Added buttons enabling removing participants from hacknight at HacknightsParticipants component.

Migrations:

N/A

New imports / dependencies:

N/A

What tests do I need to run to validate this change:

N/A

Copy link
Member

@w1stler w1stler left a comment

Choose a reason for hiding this comment

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

Seems okay. Do we have any tests for that?

@jacekkalbarczyk
Copy link
Contributor Author

We don't.

Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

It works exactly how it should - I tested PR locally, tried to break it in couple of ways and didn't accomplish shit - which is good. Nevertheless, I leave one aesthetic and some semantic comments for the sake of design and future development :D

@@ -65,6 +65,7 @@
<v-icon>mdi-account-outline</v-icon>
</v-list-item-avatar>
<v-list-item-title v-text="item.github"></v-list-item-title>
<v-btn v-on:click="onDeleteParticipant(item)">Delete</v-btn>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we consider sth more graphically aesthetic than DELETE? :D We have font awsome oboard (e.g. see the exit button in ModalContent.vue) so we could use sth like these:

Choose a reason for hiding this comment

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

Could we consider sth more graphically aesthetic than DELETE? :D We have font awsome oboard (e.g. see the exit button in ModalContent.vue) so we could use sth like these:

* https://fontawesome.com/v5.15/icons/user-times?style=solid

* https://fontawesome.com/v5.15/icons/backspace?style=solid

@OtisRed agreed - these DELETE button is kind of a psycho :D

@@ -89,6 +90,11 @@ export default {
this.$store
.dispatch('hacknight/addParticipants', ids)
.then(() => (this.selectedParticipants = []));
},
onDeleteParticipant(participant) {
const ids = [participant.id];
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably mirrored the solution for adding the participants. However @stanislawK used there ids in plural because there are many participants that can be added at once, but only one that can be deleted at a time given this solution we agreed upon. So please use sth like participant_id in singular form to name the variable and keep it consistent as you pass it to Vuex so that no one has to wonder if this is the same thing under the different names :D

@@ -76,6 +76,20 @@ export default {
.catch(error => {
commit('raiseError', error);
});
},
deleteParticipants({ commit, getters }, participants_ids) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto => deleteParticipant in singular

onDeleteParticipant(participant) {
const ids = [participant.id];

this.$store.dispatch('hacknight/deleteParticipants', ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto => participant_id

axios
.delete(`/api/hacknights/${getters.getHacknight.id}/participants/`, {
data: {
participants_ids: participants_ids
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto => participant_id: participant_id

@jacekkalbarczyk jacekkalbarczyk requested a review from OtisRed July 21, 2021 18:05
Copy link
Contributor

@OtisRed OtisRed left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left two comments regarding icon and color but I can take it how it is. I leave approve so you can decide and merge it afterwards :)

@@ -65,6 +65,9 @@
<v-icon>mdi-account-outline</v-icon>
</v-list-item-avatar>
<v-list-item-title v-text="item.github"></v-list-item-title>
<v-btn v-on:click="onDeleteParticipant(item)">
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you like this?

Suggested change
<v-btn v-on:click="onDeleteParticipant(item)">
<v-btn icon v-on:click="onDeleteParticipant(item)">

@@ -65,6 +65,9 @@
<v-icon>mdi-account-outline</v-icon>
</v-list-item-avatar>
<v-list-item-title v-text="item.github"></v-list-item-title>
<v-btn v-on:click="onDeleteParticipant(item)">
<i class="fas fa-user-times fa-lg"></i>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could also use red color to communicate deleteing function?

Suggested change
<i class="fas fa-user-times fa-lg"></i>
<i class="button_delete fas fa-user-times fa-lg"></i>

Then we could add new class:

.button_delete {
  color: $red;
}

And add global red (e.g. #ed130c) in scss/colors.scss so it can parse it from a global source :)

@OtisRed
Copy link
Contributor

OtisRed commented Jul 21, 2021

Also - you got conflicts :P

@@ -91,13 +91,13 @@ def delete(self, id):
hacknight = Hacknight.query.get_or_404(id)
participants = {participant.id for participant in hacknight.participants}
json_data = request.get_json(force=True)
ids_schema = Schema.from_dict({"participants_ids": fields.List(fields.Int())})
ids_schema = Schema.from_dict({"participant_id": fields.List(fields.Int())})
Copy link
Contributor

@stanislawK stanislawK Aug 18, 2021

Choose a reason for hiding this comment

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

We should either change participants_ids -> participant_id in corresponding unit tests, or stick with plural (which would be more consistent since in post participants_ids left)

@jacekkalbarczyk jacekkalbarczyk force-pushed the 364_Delete_participant_from_hacknight branch from f8918c5 to fe283d1 Compare August 31, 2021 13:34
@jacekkalbarczyk jacekkalbarczyk force-pushed the 364_Delete_participant_from_hacknight branch from d17739e to cc75c54 Compare August 31, 2021 13:48
@jacekkalbarczyk jacekkalbarczyk merged commit 7a1ae3f into CodeForPoznan:master Aug 31, 2021
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.

5 participants