Replace OC.Notification with toastify js

[juliusknorr]

This is a PoC pull request for #13423 to make error messages that are sent though OC.Notification more obvious and visually appealing.

Feedback is welcome @nextcloud/designers

Note that the current OC.Notification.show OC.Notification.showTemporary continue to work, but the toastify lib adds additional types (error,warning,success,info) to give a visual hint about the type of message.

ToDo as from #13423:

• Every notification needs to be dismissable
• Move the toast to the top right

[rullzer]

So the design team has to decide of course. But I'm not 100% convinced having it on the bottom is the best. All our notifications etc are on the top so that is where people look. Maybe adding it just blow the header makes more sense?

[rullzer]

Of course visually this looks a lot better than the 1990 style we had before.

[stefan-niedermann]

I think it is hard to distinguish from the other content. Maybe use a contrast color (white on dark background on normal, and dark on white background on dark theme)?

[MorrisJobke]

So the design team has to decide of course. But I'm not 100% convinced having it on the bottom is the best. All our notifications etc are on the top so that is where people look. Maybe adding it just blow the header makes more sense?

See the last open checkbox ;) It should be moved to the top right.

[juliusknorr]

I think it is hard to distinguish from the other content. Maybe use a contrast color (white on dark background on normal, and dark on white background on dark theme)?

A full colored background is a bit to much I think, but I've increased the box shadow visibility a bit (see the updated screenshot in the first post)

So the design team has to decide of course. But I'm not 100% convinced having it on the bottom is the best. All our notifications etc are on the top so that is where people look. Maybe adding it just blow the header makes more sense?

Done 😉

[jancborchardt]

Top right is indeed much better, that’s where all the "meta / organizational" stuff is. :)

And it looks really really nice! Props @juliushaertl! The only thing I can find is that the x icon is not our .icon-close icon – any reason for that?

[juliusknorr]

And it looks really really nice! Props @juliushaertl! The only thing I can find is that the x icon is not our .icon-close icon – any reason for that?

No reason, I can adjust that as well.

[juliusknorr]

@jancborchardt Thanks for the hint, pushed with the proper close icon

[skjnldsv]

Having the mouse over the notification should prevent it from disappearing I think. If you're reading something long and it disappear, you're confused :)

[juliusknorr]

Having the mouse over the notification should prevent it from disappearing I think. If you're reading something long and it disappear, you're confused :)

I'll see, but we probably need to fix that upstream.

[BrookeDot]

Having the mouse over the notification should prevent it from disappearing I think. If you're reading something long and it disappear, you're confused :)

I agree, I struggle from a bit of dyslexia and often find myself highlighting text to help me read it and sometimes reading takes a bit longer than "average". Having a notification disappear while reading it can be quite frustrating. Especially if there's not an easy way to get it back.

I'll see, but we probably need to fix that upstream.

I have opened up apvarun/toastify-js#21 upstream so we'll see if this feature can get added. If not, perhaps I can recommend not setting the timeout at all.

[skjnldsv]

@BrookeDot Thanks a lot for that! 🚀

[skjnldsv]

I have opened up apvarun/toastify-js#21 upstream so we'll see if this feature can get added. If not, perhaps I can recommend not setting the timeout at all.

Opened PR: apvarun/toastify-js#22

[karlitschek]

I like it. Looks more modern

[juliusknorr]

Do we really need that this often?
In most cases it is called to avoid that an old notification is hiding the one that should be added.

I would also be happy with just ignoring the hide method and have a default timeout of 30s for notifications that were called without a timeout though the legacy API.
Anything that speaks against this @nextcloud/designers ?

[jancborchardt]

I would also be happy with just ignoring the hide method and have a default timeout of 30s for notifications that were called without a timeout though the legacy API.
Anything that speaks against this @nextcloud/designers ?

Just to clarify: This issue does not happen when using this new notification API?

Cause a defaut timeout for notifications has a big issue in that people don’t necessarily check the screen or tab all the time, and they might miss a notification. Also, do these notifications have a close button so you can close them before the 30s?

[juliusknorr]

Just to clarify: This issue does not happen when using this new notification API?

The new API has a default timeout as I would consider those notifications to be more temporary than permanent.

Cause a defaut timeout for notifications has a big issue in that people don’t necessarily check the screen or tab all the time, and they might miss a notification. Also, do these notifications have a close button so you can close them before the 30s?

I have now also rebuild the old OC.Notifiation.hide() method, so the behavior on the old API should be complete. The only thing that has changed is the fallback if no notification to hide was provided. In that case we do nothing now, as there was a warning logged for quite some time and just removing the first notification doesn't seem to be a good fallback to me.

Let's see if the tests are happy.

[juliusknorr]

🎉 Tests are happy again. @skjnldsv@ChristophWurst Mind to have another look?

[skjnldsv]

Looks all good to me :D
Rebase, sign and squash! 🚀

[juliusknorr]

Rebase, sign and squash!

Done 🚢

[danxuliu]

@juliushaertl There is a mismatch in the options received by OC.Notification and OCP.Toast: OC.Notification uses isHTML, while OCP.Toast uses showHtml. Either OCP.Toast.message is adjusted to use isHTML too, or OC.Notification.showTemporary is adjusted to set options.showHtml based on options.isHTML (and also the unit tests to reflect that OC.Toast.message expects showHtml instead of isHTML).

[LinneyS]

If you scroll down the page, the message is not visible under the header

[ChristophWurst]

If you scroll down the page, the message is not visible under the header

Please open a new ticket.

[skjnldsv]

Should be fixed already #17110
edit: we forgot to backport