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

Bundle notifications #529

Merged
merged 12 commits into from
May 13, 2023
Merged

Bundle notifications #529

merged 12 commits into from
May 13, 2023

Conversation

mauricioszabo
Copy link
Contributor

@mauricioszabo mauricioszabo commented May 12, 2023

@confused-Techie
Copy link
Member

@mauricioszabo Can you do me a favour and add the below text into the body of your PR? It'll make things a whole lot easier when writing the ChangeLog later


* [`notifications#1 - Cleanup and rename @Sertonix `](https://github.com/pulsar-edit/notifications/pull/1)
* [`notifcations#2 - reject promise with Error instance @Sertonix `](https://github.com/pulsar-edit/notifications/pull/2)
* [`notifications#3 - Add our Testing Action @confused-Techie `](https://github.com/pulsar-edit/notifications/pull/3)
* [`notifications#4 - Change atom strings to pulsar @mdibella-dev `](https://github.com/pulsar-edit/notifications/pull/4)
* [`notifications#5 - Bump to v3.2 of action-pulsar-dependency @confused-Techie `](https://github.com/pulsar-edit/notifications/pull/5)
* [`notifications#6 - Fix all Tests @confused-Techie `](https://github.com/pulsar-edit/notifications/pull/6)

Resolves #347 

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Assuming the notification tests all pass, fully approve.
I've tried this once before and got stumped, so super excited to see it working now!

@mauricioszabo
Copy link
Contributor Author

@confused-Techie Oh.... wait a little bit....

I think I didn't merge the changes from our notification package 🤦

@mauricioszabo
Copy link
Contributor Author

Ok, done - authored the only code change on the latest commit /cc @sertonix and @confused-Techie

@confused-Techie
Copy link
Member

Awesome thanks @mauricioszabo

Also lets see if this adds it to the PR properly.

Resolves #347

@confused-Techie confused-Techie linked an issue May 12, 2023 that may be closed by this pull request
5 tasks
@mauricioszabo mauricioszabo merged commit 4a6f752 into master May 13, 2023
@mauricioszabo mauricioszabo deleted the notifications branch May 13, 2023 00:22
@sertonix
Copy link
Contributor

I forgot to check the files when it was open. My cleanup and rename pull request doesn't seem to be included eather.
Are you sure the other pull requests are?

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.

Uncaught errors still show Atom instead of Pulsar
3 participants