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

Toast notification timeout settings #2555

Conversation

BercziSandor
Copy link

@BercziSandor BercziSandor commented Jan 6, 2023

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Currently there is no time limit on error messages (20s for warnings); this behavior can be very annoying: the user has to close many notifications before being able to use the interface.
To remedy this, the following should/could be changed:

  1. Setting the timeout for toast notifications (0 or blank: no timeout). It can be set in the settings menu, separately for warnings and errors.
  2. Option to close all notification windows in one step.

Fixes #2116

Comments

  • Please update the style of the button "Delete Notifications" in file Layout.vue if you want.
  • I could not get the number of the notification messages from vue-toastification. (If there are no messages, the button should be hidden)

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@louislam
Copy link
Owner

louislam commented Jan 7, 2023

That is great. It will be accepted.

The timeout setting could be under Settings > Notifications.

@louislam louislam marked this pull request as draft January 8, 2023 06:04
@louislam louislam linked an issue Jan 9, 2023 that may be closed by this pull request
1 task
@BercziSandor
Copy link
Author

BercziSandor commented Jan 11, 2023

@louislam : I need help. (I am not an experienced JS developer)
I want to reach a value stored in settings from src/mixin/socket.js. link
How can it be done?

@chakflying
Copy link
Collaborator

Since this is only used in the browser, you can keep it simple by storing it in localStorage, like how the theme settings are kept. If you really want to keep it on the server, I guess you can take a look at how initServerTimezone is done:

  1. afterLogin (in server/server.js:1599), the server sends a command through the socket to tell the frontend to do something
  2. The frontend receive this in src/mixins/socket.js:277, and send the results needed back.

I guess you would similarly sends this setting afterLogin, and the frontend receive it and store it somewhere.

@BercziSandor BercziSandor marked this pull request as ready for review January 20, 2023 22:26
@chakflying
Copy link
Collaborator

  • If the user does not set the setting, it defaults to no timeout, which is not the previous behavior
  • The default in the UI is not the previous default, which is 20 seconds for Up and none for down
  • Personally would prefer 0 for disabled to be more consistent with other settings
  • Would be great if you can update the PR title and squash to clean up the commits.

@@ -163,7 +168,10 @@ export default {
},

methods: {

deleteNotifications() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add jsdoc for the methods you have added

@BercziSandor
Copy link
Author

  • If the user does not set the setting, it defaults to no timeout, which is not the previous behavior
    See Notifications.vue / mounted()
  • The default in the UI is not the previous default, which is 20 seconds for Up and none for down
    Fixed.
  • Personally would prefer 0 for disabled to be more consistent with other settings
    -1: no timeout: it won't disappear.
    0: no notification window: it disappears immediately
  • Would be great if you can update the PR title and squash to clean up the commits.
    Fixed.

@BercziSandor BercziSandor changed the title [empty commit] pull request for 'toast-notification-timeout-setting' Toast notification timeout settings Jan 21, 2023
@chakflying chakflying mentioned this pull request Jun 7, 2023
2 tasks
@louislam louislam added the question Further information is requested label Jun 7, 2023
@louislam louislam marked this pull request as draft June 7, 2023 16:16
@louislam
Copy link
Owner

louislam commented Jun 7, 2023

There are 68 files changed. I think something went wrong.

@CommanderStorm
Copy link
Collaborator

@louislam @BercziSandor
given that #3441 was merged, I think we can safely close this one too ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup: timestamp, timeout [Feature]: Clear All Toasts
5 participants