-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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: Show notification on import/export using interop service #10520
Desktop: Show notification on import/export using interop service #10520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
To facilitate discussion, I'm attaching a screenshot of the new UI from the linked video:
Note
It's recommended to discuss large changes on the Joplin Forum before creating a pull request, particularly for UI-related changes. This helps gather community feedback (if any) and can help determine whether a feature/change could be accepted.
Edit: There are CI errors:
Error: ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: [@joplin/app-desktop]: gui/MenuBar.tsx(295,7): error TS7034: Variable 'path' implicitly has type 'any' in some locations where its type cannot be determined.
Error: ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: [@joplin/app-desktop]: gui/MenuBar.tsx(323,12): error TS7005: Variable 'path' implicitly has an 'any' type.
packages/app-desktop/gui/InteropNotification/InteropNotification.tsx
Outdated
Show resolved
Hide resolved
When importing/exporting items in the desktop app, a giant model layer covers all our screen, and the users are forced to wait until the process is completed. This raised our attention and the need of implementing something that would let the users do their things, while still being able to know about when the process is happening and when it is finished. We though and decided that one possible solution would be using the notyf library, which right now is only used by the TrashNotification component, and show a simple and minimal notification indicating the state of the process, instead of covering the entire screen with a modal layer. We truly believe that this would drastically increase the user experience. Co-authored-by: Henrique Silva <[email protected]>
333d610
to
0380e09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a few comments.
Notes:
TaskProgressUIService
could be used in the future to resolve Implement a plugin API to display a progress bar #10527 (this would be a separate PR or commit).- I'm not sure
importOptions.onProgress
provides enough information instatus: any
to show a progress indicator (the total number of notes is unknown).- See comment below.
Thank you for working on this!
packages/app-desktop/main.scss
Outdated
display: inline-block; | ||
width: 15px; | ||
height: 15px; | ||
position: relative; | ||
|
||
border: 3px solid var(--joplin-background-color, white); | ||
border-top-color: var(--joplin-color, black); | ||
border-radius: 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display: inline-block; | |
width: 15px; | |
height: 15px; | |
position: relative; | |
border: 3px solid var(--joplin-background-color, white); | |
border-top-color: var(--joplin-color, black); | |
border-radius: 10px; | |
display: inline-block; | |
width: 15px; | |
height: 15px; | |
position: relative; | |
border: 3px solid var(--joplin-background-color, white); | |
border-top-color: var(--joplin-color, black); | |
border-radius: 10px; |
packages/app-desktop/main.scss
Outdated
|
||
.loading-spinner.-progress { | ||
width: 20px; | ||
height: 20px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
height: 20px; | |
height: 20px; |
packages/app-desktop/main.scss
Outdated
.loading-spinner.-indeterminate { | ||
animation: 4s linear infinite rotate; | ||
} | ||
|
||
@media (prefers-reduced-motion) { | ||
.loading-spinner.-indeterminate { animation: none; } | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.loading-spinner.-indeterminate { | |
animation: 4s linear infinite rotate; | |
} | |
@media (prefers-reduced-motion) { | |
.loading-spinner.-indeterminate { animation: none; } | |
} | |
.loading-spinner.-indeterminate { | |
animation: 4s linear infinite rotate; | |
} | |
@media (prefers-reduced-motion) { | |
.loading-spinner.-indeterminate { animation: none; } | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this file to packages/lib/services/
for consistency with existing code.
packages/app-desktop/gui/MenuBar.tsx
Outdated
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied | ||
const errors: any[] = []; | ||
|
||
const taskId = `interop-import-${path}}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const taskId = `interop-import-${path}}`; | |
const taskId = `interop-import-${path}`; |
packages/app-desktop/gui/MenuBar.tsx
Outdated
onProgress: (_status: any) => { | ||
TaskProgressUIService.onTaskProgress(taskId, _status); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the Evernote importer, it seems that 1) status
is an object with a created
property 2) the total number of notes to import is unknown until import completes.
For now, it might be better to show indeterminate progress for import tasks. Supporting onProgress
would still be useful for exporting (example) and #10527.
e459bc7
to
c1de532
Compare
Thank you for your help! We did what you requested above and now we also have a fully functional circular progress bar. I made a video so that you can see how it works rn. The video is just a demo of how the progress bar behaves when calling TaskUIService#onTaskProgress(). In the last commit, when importing/exporting something, the animation continues until the task is completed. notifications.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
I've reviewed the recent changes and left comments. Based on the code, this pull request seems almost ready to merge!
Based on the diff, this looks good to me! I plan to manually test the code locally in the near future (though automated tests would be welcome!). In particular, I plan to verify:
|
Closing pull request for now. Please let us know if you'd like to continue working on it |
Summary
When importing/exporting items in the desktop app, a giant model layer covers all our screen, and the users are forced to wait until the process is completed. This raised our attention and the need of implementing something that would let the users do their things, while still being able to know about when the process is happening and when it is finished.
One possible approach to this would be using the notyf library, which right now is only used by the TrashNotification component, and show a simple and minimal notification indicating the state of the process, instead of covering the entire screen with a modal layer. We truly believe that this would drastically increase the user experience.
In terms of implementation, we focused ourselves on creating a notification that stayed during the whole process, dismissing it when it's over, adding a successful notification if everything went well. For further use of other developers we created the notification based on operations, and different phases of the loading processes.
Before this feature was implemented:
Notifications after the feature was implemented:
Screencast from 05-29-2024 10 43 41 PM.webm
Testing Plan
The manual testing steps are as shown in the attached video.
Resolves #9879