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

Implement dismiss functionality for single notification #63

Merged
merged 3 commits into from
May 8, 2020

Conversation

bharatramnani94
Copy link
Contributor

  • Implement feature
  • Update doc
  • Add spec

Create a dismiss method while keeping the _removeNotification method for backward compatibility purposes (for people who might be relying on this method)

Also add .vscode to .gitignore

Closes Issue #20

Closes Issue caroso1222#20
Create a `dismiss` method while keeping the `_removeNotification` method for backward compatibilty purposes (for people who might be relying on this method)

Also add .vscode to .gitignore
src/notyf.ts Outdated Show resolved Hide resolved
@bharatramnani94 bharatramnani94 marked this pull request as ready for review April 11, 2020 11:43
@caroso1222
Copy link
Owner

Wow, amazing work, thank you! Weekend is wife time but I'll take a look at this first thing on Monday. Thanks again 😃

Just one thing real quick, I see you're adding explicit return types to some functions. I prefer implicit type inference as it makes the code less verbose while still keeping it safe and allowing intellisense. I propose a middle ground, though: enable the flag no-inferrable-types in tslint.json and add types only when they can't be inferred. How's that sound to you?

@bharatramnani94
Copy link
Contributor Author

I personally prefer explicit return types (particularly in a public codebase) so that I'm aware while changing the API (for an existing method), and that its a conscious decision. This becomes even more crucial when we don't have unit tests that check a particular method in isolation, and prevents cases when we simply forget about the return type and unintentionally change it.

Having said that, what you described also makes sense, and I'm more than happy to update the PR - just let me know.

@caroso1222
Copy link
Owner

Yes please! if you can update the PR with no-inferrable-types that'd be great. If you're short on time, I can also take it. Let me know.

demo/scripts/script-test.js Show resolved Hide resolved
cypress/integration/notyf_spec.js Show resolved Hide resolved
cypress/integration/notyf_spec.js Show resolved Hide resolved
@caroso1222
Copy link
Owner

@bharatramnani94 I've put some more comments in regards to the testing strategy. I think that's a lot of work for you, feel free to drop the responsibility on me. I can make those changes, but I'll leave the call on you.

Thanks again for all the help

Copy link
Owner

@caroso1222 caroso1222 left a comment

Choose a reason for hiding this comment

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

I completed what was missing. Thanks for your contribution!

@caroso1222 caroso1222 merged commit 2de6f70 into caroso1222:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants