Add close button to notifications #18
Conversation
|
Just pushed the last changes for overriding the close button setting per-notification, and also rewritten some details in the documentation to match the new changes on the notification API, if something seems off or out of place, let me know and i'll take a look at it as soon as possible |
ghost
requested a review
from 
|
@antony I'd really appreciate this! |
|
I'll try to review and merge this as soon as possible! Sorry about the delay! |
There was a problem hiding this comment.
Apologies it took me nearly a year to review this! I've been a little busy, shall we say.
I think the changes are great but there are 3 or 4 PRs in here rather than one, which makes it hard to review + if I disagree with one part it blocks me merging the whole lot.
I don't know if you're still invested in getting these changes in, but if so, I'd suggest raising the new feature as a PR on its own. We should probably make the timeout optional if you can close notifications.
| @@ -126,13 +126,13 @@ You can set a timeout per message | |||
| import { NotificationDisplay, notifier } from '@beyonk/svelte-notifications' | |||
|
|
|||
| function someFunction () { | |||
| notifier.success('Notifications work!', 5000) // built in theme | |||
| notifier.send('custom-theme', 'Notifications work!', 5000) // custom theme | |||
| notifier.success('Notifications work!', { duration: 5000 }) // built in theme | |||
There was a problem hiding this comment.
Unfortunately this significantly changes the core API. I like the idea of an options object in principle, but this decision should be made in a completely separate PR which only modifies the API, not muddled in among some refactorings and a new feature.
| } | ||
| </script> | ||
| ``` | ||
|
|
||
| ## Credits | ||
|
|
||
| * Original code by [Antony Jones](https://github.com/antony) | ||
| * Animation and performance improvements by jg. | ||
| - Original code by [Antony Jones](https://github.com/antony) |
There was a problem hiding this comment.
This just clutters the PR and detracts from the feature you've added.
| @@ -0,0 +1,13 @@ | |||
| <script> | |||
| import { NotificationDisplay, notifier } from '../src' | |||
There was a problem hiding this comment.
I wasn't able to get the demo app to run. Is it worth adding a readme to tell people how to run it?
| "svelte": "^3.24.1" | ||
| "@rollup/plugin-commonjs": "^15.1.0", | ||
| "@rollup/plugin-node-resolve": "^9.0.0", | ||
| "rollup": "^2.29.0", |
There was a problem hiding this comment.
Upgrading all the libraries as part of a PR for a feature isn't advisable. It's a great idea to do this, but it should be in an isolated PR on its own.
| { file: pkg.main, 'format': 'umd', name } | ||
| ], | ||
| plugins | ||
| input: 'src/Notifications.svelte', |
There was a problem hiding this comment.
It's best to keep formatting changes out of PRs as it makes it hard to determine what the real changes are.
| @@ -3,7 +3,12 @@ | |||
| <li class="toast" style="background: {toast.background};" out:animateOut> | |||
| <div class="content"> | |||
| {toast.msg} | |||
| </div> | |||
| </div> | |||
| {#if toast.closeable} | |||
There was a problem hiding this comment.
Looks like a good addition - I think closable toasts should probably be allowed without a timeout too, so that they have to be closed. Not sure how we'd express that in the API.
| let unsubscribe | ||
|
|
||
| function animateOut(node, { delay = 0, duration = 1000 }) { | ||
| function vhTOpx (value) { | ||
| var w = window, |
There was a problem hiding this comment.
weird, not sure why this dead code was in there!
|
Thanks for your feedback @antony, i'll look towards it maybe later today or tomorrow, seems like a bunch of stuff needs to be improved, anyways, this was made almost an year ago (surely i wasn't the best open-source contributor back then 😆 ) |
|
@henriquehbr no worries. Closing this, and I'll implement some of your ideas separately :) |
This PR aims to implement a bunch of small additions, most of them are refactorings:
exampledirectory in order to test and preview the componentNotifications.svelteandstore.jsfunctions to arrow functionsAll the changes were manually tested multiple times in order to assure that they won't break or conflict with existing features