-
Notifications
You must be signed in to change notification settings - Fork 20
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
Notifications rewrite + small fixes #556
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.
Looks great! Left a few nits, and style/lints need to be fixed.
I realized that there's a lot of duplicated code with Svelte now. Since the component seems to work well, let's just nuke the old Svelte code and notifications code, to avoid having two copies there.
But please still keep the newUI
logic, we will use it in the future for more complicated components.
I will do some push notification fixed on top of this PR (#557) once this is merged. |
I just looked into svelte files, and we can only remove Notifications.svelte file, becuase notifications.js is also used here |
Just out of curiosity, is Svelte (+ our build system) unable to import code from |
Okay, so importing typescript files into svelte works, so I removed the old js and svelte file and changed the import in TaskDetails and also tested it. |
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.
Ok, awesome :) Thank you, this is great!
X-CSRFToken
and then call getFromAPI. This is alternative to fetch function in api.js (also swapped order of parameters of getFromAPI from url, data, method, headers to url, method, data, headers, beacause we usually want to specify method first and then data (if any).