-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Release/168.0.0 #4469
Release/168.0.0 #4469
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.
LGTM!
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.
Hi, I add some suggestions on this.
d01a985
to
801d009
Compare
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.
LGTM :)
013e2f7
to
1898a91
Compare
adds more comprehensive changelog for these changes
|
||
- export `NOTIFICATION_NETWORK_CURRENCY_NAME` and `NOTIFICATION_NETWORK_CURRENCY_SYMBOL`. Allows consistent currency names and symbols for supported notification services ([#4441](https://github.com/MetaMask/core/pull/4441)) | ||
|
||
- added `isPushIntegrated` optional env property (defaults to true) ([#4441](https://github.com/MetaMask/core/pull/4441)) |
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 good! Just had one last addition for clarity:
- added `isPushIntegrated` optional env property (defaults to true) ([#4441](https://github.com/MetaMask/core/pull/4441)) | |
- add `isPushIntegrated` as an optional env property in the `NotificationServicesController` constructor (defaults to true) ([#4441](https://github.com/MetaMask/core/pull/4441)) |
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.
Merged!
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.
Silly question - but do we have a changelog "best practices" doc or notes? E.g. past tense, be descriptive, etc
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.
No that's a good point. Closest thing we have is this: https://github.com/MetaMask/core/blob/main/docs/reviewing-release-prs.md (especially section 5), but it could definitely be more comprehensive.
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.
Nice!
Co-authored-by: Jongsun Suh <[email protected]>
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.
LGTM!
This reverts commit 879280b.
Reverts #4469 We need to redo this release because the version was not set on the `package.json` and did not go out 🤦🏾 , apologies!
Explanation
This PR create a new release for
@metamask/profile-sync-controller
to^0.2.0
@metamask/notification-services-controller
to^0.2.0
References
Fixes mobile integration issues.
Changelog
@metamask/profile-sync-controller
@metamask/notification-services-controller
Checklist