-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(cloudwatch): add option to show labels on pie chart #34745
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
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.
(This review is outdated)
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
f2c794c to
b120954
Compare
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| * | ||
| * @default false | ||
| */ | ||
| readonly displayLabelsOnChart?: boolean; |
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.
Since this applies specifically to pie charts, the name can be more specific:
| readonly displayLabelsOnChart?: boolean; | |
| readonly displayLabelsOnPieChart?: boolean; |
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.
I think this depends on how confident we are that cloudwatch won't add more widgets in the future that have the same option to show labels on the graph. Ideally we would have something like a PieGraph class so we could have props that only make sense for pie charts. I think that would be too big of a change for this feature, though.
I plan on just adding the validation and extra unit test. Please let me know if you disagree, and if we should still move forward with the rename.
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.
I think this depends on how confident we are that cloudwatch won't add more widgets in the future that have the same option to show labels on the graph
I did think about, but I saw the comment above stating that it is for Pie charts only, which is what prompted the suggestion.
I think it's fine to keep the current naming, but let's update the JSdoc to clarify that this only works for Pie charts for now.
| }]); | ||
| }); | ||
|
|
||
| test('add displayLabelsOnChart to GraphWidget', () => { |
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.
Let's add a test which verifies what happens when the graph type is not a pie chart.
|
Hi @Kasra-G , just checking if you need any further feedback/support to update this PR. |
|
Thanks for the check-in. Haven't had a chance to make the changes yet, I do intend on getting that done before the PR gets marked stale |
b120954 to
94295c3
Compare
Pull request has been modified.
|
Not sure why Edit: Force pushed back to the previous version and I guess the check is still failing. Not sure what I can do, my guess is that the check itself is broken given I see several other open PRs with the same error |
94295c3 to
b120954
Compare
|
@vishaalmehrishi I have made the appropriate changes but for some reason the PR code build is not rerunning and I am getting some file not found error in the I noticed my other PR is also not running the PR code build, did that change recently? |
|
Seems the issue might be that pr code build only runs for branches targeting |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
This pull request has been removed from the queue for the following reason: The merge conditions cannot be satisfied due to failing checks:You may have to fix your CI before adding the pull request to the queue again. |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
@Mergifyio requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #28929
Reason for this change
Cloudwatch Dashboard console has this option but cdk
GraphWidgetdoes notDescription of changes
GraphWidgetPropscalleddisplayLabelsOnChartDescribe any new or updated permissions being added
N/A
Description of how you validated changes
Created new unit and integration test and ran them
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license