-
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
feat(scatter): Add Scatter to the Angular package and storybook #301
feat(scatter): Add Scatter to the Angular package and storybook #301
Conversation
Deploy preview for carbon-charts-core ready! Built with commit c6f268c |
Deploy preview for carbon-charts-vue ready! Built with commit c6f268c |
Deploy preview for carbon-charts-react ready! Built with commit c6f268c |
Deploy preview for carbon-charts-angular ready! Built with commit c6f268c https://deploy-preview-301--carbon-charts-angular.netlify.com |
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, except for 1 thing 💯
* Most functions just call their equivalent from the chart library. | ||
*/ | ||
@Component({ | ||
selector: "n-scatter-chart", |
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.
Totally never caught this ... we'll want to switch the prefix to either ibm-
(to match carbon-angular) or c-
(for carbon) ... the easiest way going forwards will be "ibm-scatter-chart, n-scatter-chart"
(or whatever prefix) until we hit v1.
Doesn't stop this PR, but just something we should sort out sooner rather than later.
thanks @stanislavgeorgiev |
Cool, my first contribution to Charts 👍 |
Updates
Demo screenshot or recording
Review checklist (for reviewers only)