-
Notifications
You must be signed in to change notification settings - Fork 541
[core-v2.3.1-alpha.4, react-v2.2.1-alpha.4, vue-v2.1.1-alpha.3] : Update - Decouple Notify and AC positioning #1098
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
[core-v2.3.1-alpha.4, react-v2.2.1-alpha.4, vue-v2.1.1-alpha.3] : Update - Decouple Notify and AC positioning #1098
Conversation
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
…knative/onboard into v2-web3-onboard-develop
|
|
||
| export type NotifyOptions = { | ||
| desktop: Notify | ||
| mobile: Notify |
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.
@Adamj1232 Could we possible discuss naming conventions here? Notify vs NotifyOptions
why are they separate? and then possibly leave the Notify namespace open in the event that we decouple notify from w3o and need to use the Notify type? Just a thought
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.
@taylorjdawson I see what you are saying, I was following the pattern we have used for AccountCenter where ...Options are passed in on init and Notify in this case is used internally for Notify configurations. If we were to decouple I dont see a challenge and updating the types but def want to have similar patterns with the different components
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.
Okay no worries, is there a way to make the name more descriptive? When I saw the Notify type I thought it excepted a notify instance.
Description
This PR is to allow the decoupling of the Account Center and Notifications.
To do this the initialization type of Notifications needed to expand to allow similar desktop and mobile positioning in the same pattern as Account Center.
This was developed with backwards compatibility in mind so initialization allows NotifyOptions as well as Notify that the initial release was set to accept and validate within the init.
Checklist
package.jsonis incremented following semantic versioning