-
Notifications
You must be signed in to change notification settings - Fork 364
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-20050] - Added Data Rollup logic for graph data #10747
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, this is the same changes we have in our integrated branch
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
@jaalah-akamai thanks for the suggestions. I've updated those & added test cases |
@jaalah-akamai, yes this is being addressed in the separate PR which @venkymano is working on with configured global filters. Along with responsiveness for different device support. |
Coverage Report: ✅ |
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 feedback on initial pass through and will re-review once that feedback has been addressed.
packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
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
@mjac0bs Thanks for the comments. I've addressed them and also created new changesets with upcoming-feature tag |
packages/manager/.changeset/pr-10747-upcoming-features-1723026263446.md
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/CloudPulseWidget.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
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.
Thanks for addressing feedback @nikhagra. Error and loading states look cleaner and comments for documentation help everyone.
I've provided more detail (and hopefully more clarity) in comments about what we'd like to see for util functions with several parameters. Once those are cleaned up, this is looking in better shape to be merged.
A note to address in a future PR: there is a typo in the cloudpulse query params so they read: Clousepulse
. (We also generally lowercase these, so cloudpulse
would be standard here.)
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
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
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx
Outdated
Show resolved
Hide resolved
legendRows={ | ||
legendRows && legendRows.length > 0 ? legendRows : undefined | ||
} | ||
ariaLabel={props.ariaLabel ? props.ariaLabel : ''} |
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.
Same comment as below - please destructure your props.
ariaLabel={props.ariaLabel ? props.ariaLabel : ''} | |
ariaLabel={ariaLabel ?? : ''} |
packages/manager/src/features/CloudPulse/Utils/unitConversion.test.ts
Outdated
Show resolved
Hide resolved
@mjac0bs @dwiley-akamai I've addressed comments & enhanced jsDocs. Please check & approve if all good |
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.
Approving since most of feedback clean up has been addressed, but left some final bits of feedback for you, @nikhagra. 👍🏼
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/CloudPulse/Utils/CloudPulseWidgetUtils.ts
Outdated
Show resolved
Hide resolved
@mjac0bs thanks for the approval, I've updated the interface structure. @jaalah-akamai @mjac0bs @dwiley-akamai we've received required approval, please merge. |
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.
Code review ✅
Mocked graphs ✅
legendRows={ | ||
legendRows && legendRows.length > 0 ? legendRows : undefined | ||
} | ||
ariaLabel={ariaLabel ? ariaLabel : ''} |
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.
could use a nullish coalescing operator here
ariaLabel={ariaLabel ? ariaLabel : ''} | |
ariaLabel={ariaLabel ?? ''} |
The only CI failure was on I'm going to merge this PR now. |
Description 📝
Added data rollup logic to convert the graph data to highest possible unit based on the base unit of data we receive from widget configuration.
Changes 🔄
List any change relevant to the reviewer.
Target release date 🗓️
15th August 2024
Preview 📷
Include a screenshot or screen recording of the change
💡 Use
<video src="" />
tag when including recordings in table.How to test 🧪
Note: You may notice some flickering because of continuous re-rendering but that will be resolved once global filter change handler will be implemented.
Check all that apply