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

Merge request for displaying notes count to the side of folder name in electron. #577

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
3 participants
@ghost
Copy link

commented May 31, 2018

  1. Added the element to SideBar.jsx for displaying the notes count present in that particular folder.
  2. Created a file called note-count-utils.js which is used for updating the notes count when ever there is action done on notes like adding a note, deleting a note.
  3. Calling a notes count refresh method from NoteList when the note is deleted.
  4. Calling a notes count refresh method from Mainscreen when a new notebook is added.
  5. Assigning the store dispatch to note-count-utils dispatch from BaseApplication.js.
  6. Added a new action type FOLDER_COUNT_UPDATE_ALL in the reducer and modified FOLDER_UPDATE_ALL action type to save folder count to new state.

This is a PR for enhancement of #518 in electron.

Please see the below image with enhancement

notes-count-electron

karthik
1. Added the element to SideBar.jsx for displaying the notes count in…
… that particular folder.

2. Created a file called note-count-utils.js which is used for updating
the notes count when ever there is action done on notes like adding a
note, deleting a  note.
3. Calling a notes count refresh method from NoteList when the note is
deleted.
4. Calling a notes count refresh method from Mainscreen when a new
notebook is added.
5. Assigning the store dispatch to note-count-utils dispatch from
BaseApplication.js.
6. Added a new action type FOLDER_COUNT_UPDATE_ALL in the reducer and
modified FOLDER_UPDATE_ALL action type to save folder count to new state.
@foxmask

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2018

look nice !

@laurent22

This comment has been minimized.

Copy link
Owner

commented Jun 1, 2018

There are issues with white spaces here too unfortunately. Please limit the commit to only the code changes.

Otherwise, I wonder about the performance of this change? It seems it's looping through all the folders to count the notes every time it needs to be refreshed. Shouldn't there be instead just one SQL query to get all the data in one go?

Should the data be cached?

Also it seems NoteCountUtils.refreshNotesCount() is manually called everywhere the note count might change, but there are other ways in which the note count can change, such as via sync, with the web clipper or when a note is moved to a different notebook. I think this refreshNotesCount() should be called based on events, either in the Redux middleware or by creating a service a bit similar to ResourceService which would loops through the recent changes in item_changes and update the note count accordingly.

@ghost ghost changed the title Merge request for displaying notes to the side of folder name in electron. Merge request for displaying notes count to the side of folder name in electron. Jun 6, 2018

@foxmask

This comment has been minimized.

Copy link
Collaborator

commented Jun 26, 2018

that'd be cool if the same was applied for tags :)

@laurent22

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2018

Closed abandoned pull request.

@laurent22 laurent22 closed this Sep 3, 2018

@foxmask

This comment has been minimized.

Copy link
Collaborator

commented Sep 3, 2018

:(

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

@laurent22 dumb question, but why was this closed? The second commit (at least from the commit message) addressed your comments.

@laurent22

This comment has been minimized.

Copy link
Owner

commented Jul 9, 2019

The person who started the PR is gone, and I can see there's still work to do but unfortunately noone to do it.

@tessus

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

Ah, ok. Too bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.