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

Desktop: Resolves #9984: Allow 'All Notes' to have 'Toggle own sort order' #10021

Merged
merged 30 commits into from Mar 16, 2024

Conversation

HahaBill
Copy link
Contributor

@HahaBill HahaBill commented Feb 28, 2024

Summary

Introduction from the Joplin Forum: https://discourse.joplinapp.org/t/introducing-hahabill/36075/3

The user can 'Toggle own sort order' for 'All notes', this is demonstrated in the video below.

Fixes: #9984

Testing and Verification Process

1. To ensure the changes are working as expected:

Screen.Recording.2024-02-28.at.17.14.01.mov

2. Testing:

If more tests are needed, let me know!

Notes

I noticed that you are not able to custom ordering with 'All Notes', not sure if this behavior is desired or not:

Screen.Recording.2024-02-28.at.18.05.52.mov

Checklist:

  • Fixed the issue
  • Working PR

HahaBill and others added 10 commits February 22, 2024 23:52
…e bar

- Fixed the issue laurent22#7834

- added "spellCheckerMenu.test.ts" to test the changes and the expected behavior: all passed!

- added jest.mock in "jest.setup.js" to mock the Menu from @electron/remote.

-  modified "buildScriptIndexes" to ignore ".test.ts" files so it does not include them in index.ts. That caused some issues with duplicate identifiers and more errors.
- Test case passed: ['en-GB', 'en-US', 'en-CA', 'es-ES', 'es-MX'] -> 'en, es'
- manually tested locally and the changes work!
- major change: added the 'All Notes' folder when initializing the app and implemented the 'folderExists' to prevent adding the 'All Notes' again.
@HahaBill HahaBill marked this pull request as ready for review February 28, 2024 18:26
@HahaBill
Copy link
Contributor Author

Something's wrong here, only 1 check and merge conflicts with my already merged commits

@HahaBill
Copy link
Contributor Author

HahaBill commented Mar 1, 2024

@PackElend label me please

Copy link
Owner

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. The code is cleaner and more compact now, but please see the comments below.

packages/app-desktop/gui/Sidebar/Sidebar.tsx Outdated Show resolved Hide resolved
Comment on lines 476 to 477
folderItem_click(ALL_NOTES_FILTER_ID);
};
Copy link
Owner

Choose a reason for hiding this comment

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

While we're at it, would you mind making this onAllNotesClick_ handler use useCallback too? It would have folderItem_click and props.dispatch as dependencies

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 don't mind at all, I'll do it!

@laurent22
Copy link
Owner

Looks good now, thanks @HahaBill!

@laurent22 laurent22 merged commit 7638bdf into laurent22:dev Mar 16, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to "Toggle own sort order" should be added to "All notes"
3 participants