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: notes | Multiple Categories deletion feature #770

Closed
wants to merge 5 commits into from

Conversation

Jazzxe
Copy link

@Jazzxe Jazzxe commented Apr 30, 2024

#561

Fixes Issue

[FEATURE] Notes App, Manage Selecting Multiple Notes. #561

Changes proposed

Added the checkbox next to notes that when checked allocates the state to selected, triggers the button delete all to appear. Deletion happens onClick and after confirmation in a popup window.

Screenshots

Screenshot 2024-05-01 003132
Screenshot 2024-05-01 003140
Screenshot 2024-05-01 003155
Screenshot 2024-05-01 003414

Note to reviewers

Code of Conduct

  • [ ✓] By submitting this pull request, I confirm I've read and complied with the CoC 🖖

Check List (Check all the applicable boxes)

  • [✓ ] My code follows the code style of this project.
  • [ ✓] My change requires changes to the documentation.
  • [ ✓] I have updated the documentation accordingly.
  • [✓ ] All new and existing tests passed.
  • [✓ ] This PR does not contain plagiarized content.
  • [ ✓] The title of my pull request is a short description of the requested changes.

You can also join our Discord community.
Feel free to check out other cool repositories of the Thecyberworld.
Join the Thecyberworld GitHub Organisation by raising an issue (you will be sent an invitation).

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

It's great having you contribute to this project

🌟 Welcome to the community 🌟

  • If you would like to continue contributing to open source and would like to do it with an awesome inclusive community.
  • You should join our Discord chat and our GitHub Organisation.
  • We help and encourage each other to contribute to open source little and often 😄.
  • Any questions let us know.

@kabir0x23
Copy link
Member

is checkbox will be default?

we can add a option of edit on top of the screen, then maybe we can see those opttions like checkbox in all notes

@ArkadiK94 what to do you think

@ArkadiK94
Copy link
Collaborator

@kabir0x23 maybe on holding the ctrl button on the keyboard, the user could pick many notes, and in this time will appear the "delete notes" button 🤔 without checkboxs

But your idea is easier to implement, in my opinion, so we can go with it.

@Jazzxe
Copy link
Author

Jazzxe commented May 1, 2024

@kabir0x23 yes at the moment the checkbox appears by default, your approach makes total sense but as I implemented the delete all button to appear only after selection, adding and edit button then selecting the element in order to see the delete all will be a bit confusing to users I think.

However @ArkadiK94 in my opinion , it is unusual to use "crtl + lmb" in order to select multiple things on a web application, I think users wont think about it in order to use it as it is not a built habit.

How about redesigning for it to be less visually space-occupying this way.

Screenshot 2024-05-01 143730

@ArkadiK94
Copy link
Collaborator

ArkadiK94 commented May 1, 2024

@Jazzxe Instead of edit button, you can at the bottom add toggle "on | off" button. When "on" you can show immediately the "delete notes" even when the user didn't select any note. With appropriate label.

@kabir0x23 what do you think?

@kabir0x23
Copy link
Member

@Jazzxe Instead of edit button, you can at the bottom add toggle "on | off" button. When "on" you can show immediately the "delete notes" even when the user didn't select any note. With appropriate label.

@kabir0x23 what do you think?

nice idea, we can ready any simple base for now

later we can set a new value in noites, isTrashed: true

then we can show all of the deelted into trash route

@ArkadiK94 ArkadiK94 changed the title Multiple Notes deletion feature feat: notes | Multiple Notes deletion feature May 2, 2024
@ArkadiK94
Copy link
Collaborator

@Jazzxe you committed the deletion of file_upload file to the git, needs to add it back. your antivirus deleted it but you should not commit and stage it. Can you add it back or do you need help with it?

@Jazzxe
Copy link
Author

Jazzxe commented May 2, 2024

@ArkadiK94 okay I'll work on the proposed idea .
However, I know how to add only If I open another pull request , if you have any better solution I would want some guidance. Also about the deleted file, can you explain what is it exactly, from my basic understanding I can see it is a backdoor threat which is obviously something I do not want.

@ArkadiK94
Copy link
Collaborator

@Jazzxe Yes, you don't want or need it to have on your local computer for this reason the antivirus deleted it. However, we use it in the website so next time you should not commit it, you need to commit only your changes.
I will fix this for now, I only add this file back and commit the change again.

@ArkadiK94
Copy link
Collaborator

@Jazzxe the file contains malicious data templates, it's only templates not real malicious data, it's like dummy data,

that's why antivirus removed it.

why: this file is needed in the website
@ArkadiK94
Copy link
Collaborator

@Jazzxe I restored the file :)

@Jazzxe
Copy link
Author

Jazzxe commented May 3, 2024

@ArkadiK94 @kabir0x23 About the toggle button what do you think its label should be and do you prefer the square or round checkbox design ?

@ArkadiK94
Copy link
Collaborator

@Jazzxe In my opinion, "multiselect" and round

@ArkadiK94
Copy link
Collaborator

@Jazzxe I will change it to draft tell us when it will be ready for review

@ArkadiK94 ArkadiK94 marked this pull request as draft May 8, 2024 20:53
@Jazzxe
Copy link
Author

Jazzxe commented May 9, 2024

I was busy last week didn't have much time to work on this, I added the multiselect in the bottom of the list as you suggested, it looks like this in thoggled status , untoggled "Delete All" , and checkboxes are not visible:
Screenshot 2024-05-09 030022

@ArkadiK94
Copy link
Collaborator

ArkadiK94 commented May 9, 2024

@Jazzxe "Multi Select" and is it only a button?
What do you think about making it a switch button?
Screenshot_20240509_081938_Firefox.jpg

@Jazzxe
Copy link
Author

Jazzxe commented May 9, 2024

I don't know it that would be appropriate for this feature. How about only the color of the label and icon changes depends on status ? meaning the icon and "multiselect" will render green / red when its toggled / toggled off ?

@ArkadiK94
Copy link
Collaborator

@Jazzxe do you think it is enough indication for the user to understand that this is turned on or off?

@kabir0x23
Copy link
Member

@Jazzxe do you think it is enough indication for the user to understand that this is turned on or off?

we can keep it with funcnality and with any simple way, i will work on design later

@ArkadiK94 ArkadiK94 marked this pull request as ready for review May 10, 2024 09:05
@ArkadiK94
Copy link
Collaborator

@Jazzxe Hmm, is it confusing where are the categories and where are the notes? because you implemented the deletion for categories and not notes...

Recording.2024-05-11.181040.mp4

@ArkadiK94
Copy link
Collaborator

@Jazzxe it is good on one hand because we need such functionality for categories but also for notes...

@ArkadiK94 ArkadiK94 changed the title feat: notes | Multiple Notes deletion feature feat: notes | Multiple Categories deletion feature May 11, 2024
@ArkadiK94
Copy link
Collaborator

ArkadiK94 commented May 11, 2024

@Jazzxe better to make it clickable when we click in every place on the category name.

Recording.2024-05-11.182414.mp4

@ArkadiK94
Copy link
Collaborator

ArkadiK94 commented May 11, 2024

@Jazzxe When I select category click on "MultiSelect" Then click it again. I don't see that the categories selected but when I click on delete they are deleted.

Recording.2024-05-11.184651.mp4

@Jazzxe
Copy link
Author

Jazzxe commented May 11, 2024

I think I got confused yes, because it says "Notes" on the left side of the table, but now I understand that you call those categories which have notes. And even in the first screenshots I provided it was clear I was working on the left side.
So do you want to keep the feature for the "categories" and add it also to the "notes" ?
Screenshot 2024-05-11 164659

Copy link
Collaborator

@ArkadiK94 ArkadiK94 left a comment

Choose a reason for hiding this comment

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

@Jazzxe Thanks for your contribution :)
Please, fix the comments.

}
});
// RemoveSelected note
export const RemoveSelectedCategory = createAsyncThunk("notesCategory/remove", async (id, thunkAPI) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as I wrote upwards for AddSelectedCategory function

@@ -56,12 +57,56 @@ export const deleteNotesCategory = createAsyncThunk("notesCategory/delete", asyn
return thunkAPI.rejectWithValue(message);
}
});
// IsSelected user category: note
export const AddSelectedCategory = createAsyncThunk("notesCategory/selected", async (id, thunkAPI) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const AddSelectedCategory = createAsyncThunk("notesCategory/selected", async (id, thunkAPI) => {
export const addSelectedCategory = createAsyncThunk("notesCategory/selected", async (id, thunkAPI) => {

I didn't understand why you use createAsyncThunk for a non async function

@ArkadiK94
Copy link
Collaborator

ArkadiK94 commented May 11, 2024

I think I got confused yes, because it says "Notes" on the left side of the table, but now I understand that you call those categories which have notes. And even in the first screenshots I provided it was clear I was working on the left side. So do you want to keep the feature for the "categories" and add it also to the "notes" ?

Yes, in this pr, it will be only for categories, in next pr, you can do for notes as well.

If you think we should change the title or add another one to make the sections more clear I would be happy to hear.

@ArkadiK94
Copy link
Collaborator

@Jazzxe I see that you resolved some comments. How is it going?

@ArkadiK94
Copy link
Collaborator

@Jazzxe any update?

@ArkadiK94 ArkadiK94 self-assigned this May 31, 2024
@ArkadiK94
Copy link
Collaborator

see pr #858

@ArkadiK94 ArkadiK94 closed this Jun 11, 2024
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

3 participants