-
Notifications
You must be signed in to change notification settings - Fork 357
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
upcoming: [DI-19818] - Added line graph to CloudPulse widgets for different metrics #10710
upcoming: [DI-19818] - Added line graph to CloudPulse widgets for different metrics #10710
Conversation
Coverage Report: ✅ |
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetColorPalette.ts
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
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.
Left some initial feedback to be addressed. I'll continue to review but this should give you a starting point. Reach out if you have questions.
Sure thanks for comments, I'll addressed them and also put bigger logic into a util file and reduce the component size |
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
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/shared/CloudPulseTimeRangeSelect.tsx
Show resolved
Hide resolved
There seems to be a lot of jankiness when you change values for the charts, I'm not sure if that's because we're dealing with mocks or if it's due to multiple re-renderings. It's just something to keep an eye on as development progresses 👍 |
Approving pending changes request from Banks |
ddd9ce1
to
ab0a446
Compare
Looks like you have a test failure for |
@carrillo-erik this should be ready for another pass 👍 |
@jaalah-akamai it seems some other test is failing as it doesn't belong to ACLP |
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx
Outdated
Show resolved
Hide resolved
I highly recommend that the CloudPulse use |
We have already discussed regarding this & for now we can move with chart.js for our beta release. |
@nikhagra Could you merge in the latest changes from |
40821c1
to
acd515f
Compare
@jdamore-linode updated from latest dev branch |
I'm aware of that Slack discussion, but I don't agree with the outcome. It's just creating more work for us later. @nikhagra |
Its in the radar already post discussion on the slack , we will be migrating it post beta asap. Hope this shouldn't be a showstopper for this PR and beta which we have a very tight-timeline of 2 more weeks only for dev integrations with service owners. Thanks!! |
Thanks!! @bnussman-akamai for the approvals. Can this request be merged now please? |
@jaalah-akamai @jdamore-linode @bnussman-akamai I've marked marked all the conversations as resolved but still it says merging blocked because of requested changes. Could you please check and merge |
Description 📝
Line graph component is added in cloud pulse widget to showcase data for different metrics.
Changes 🔄
List any change relevant to the reviewer.
Target release date 🗓️
2nd August 2024
Preview 📷
Include a screenshot or screen recording of the change
💡 Use
<video src="" />
tag when including recordings in table.How to test 🧪
As an Author I have considered 🤔
Check all that apply