Skip to content

feat: adding gauge widget #347

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

Merged
merged 4 commits into from
Nov 11, 2020
Merged

feat: adding gauge widget #347

merged 4 commits into from
Nov 11, 2020

Conversation

anandtiwary
Copy link
Contributor

@anandtiwary anandtiwary commented Nov 10, 2020

Description

Please include a summary of the change, motivation and context.
Adding Gauge Dashboard widget

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

Added unit tests for both widget and its model. Also did some manual test. We need to ensure the widget renders the data correctly.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.



// Gauge
export * from './shared/components/gauge/gauge.component';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubled up gauge component/module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

import { DonutWidgetRendererComponent } from './gauge-widget-renderer.component';
import { DonutWidgetModel } from './gauge-widget.model';

describe('Donut widget renderer component', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing this is not ready for review

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready now

@anandtiwary anandtiwary marked this pull request as ready for review November 11, 2020 19:24
@anandtiwary anandtiwary requested a review from a team as a code owner November 11, 2020 19:24
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #347 (40ab3b9) into main (85d4688) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #347      +/-   ##
==========================================
+ Coverage   86.23%   86.25%   +0.02%     
==========================================
  Files         725      728       +3     
  Lines       14807    14831      +24     
  Branches     1760     1896     +136     
==========================================
+ Hits        12769    12793      +24     
+ Misses       2007     2005       -2     
- Partials       31       33       +2     
Impacted Files Coverage Δ
projects/components/src/public-api.ts 100.00% <ø> (ø)
...ity/src/shared/components/gauge/gauge.component.ts 95.12% <ø> (ø)
projects/observability/src/public-api.ts 100.00% <100.00%> (ø)
...bility/src/shared/components/gauge/gauge.module.ts 100.00% <100.00%> (ø)
...d/widgets/gauge/gauge-widget-renderer.component.ts 100.00% <100.00%> (ø)
...ared/dashboard/widgets/gauge/gauge-widget.model.ts 100.00% <100.00%> (ø)
...red/dashboard/widgets/gauge/gauge-widget.module.ts 100.00% <100.00%> (ø)
.../widgets/observability-dashboard-widgets.module.ts 100.00% <100.00%> (ø)
...pover/position-strategy/mouse-position-strategy.ts 9.67% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 85d4688...40ab3b9. Read the comment docs.

const modelFactory = createModelFactory();

const data: GaugeWidgetData = {
value: 5,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm - I think I'd expect the max and thresholds to come from the widget model rather than the data model - those seem like they're independent of where we actually get the value from, and would allow dropping in our existing aggregation data sources.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will follow up on the data/widget separation.

@anandtiwary anandtiwary merged commit ba18a38 into main Nov 11, 2020
@anandtiwary anandtiwary deleted the add-gauge-widget branch November 11, 2020 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants