Skip to content
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

Add gauge visualization #4074

Closed

Conversation

IdoPeles
Copy link

@IdoPeles IdoPeles commented Aug 15, 2019

What type of PR is this? (check all applicable)

  • Feature

Description

This if my first time contributing to an open source project , please excuse my mistakes and let me know what should I do next if needed

This feature is a simple arc displaying 3 colors and a value used to describe status
value should be passed in column called "value" result set should return one row

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

gauge

Copy link
Collaborator

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

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

Hi @IdoPeles 👋 Thank you for your contribution to the project - we're always happy to see new visualizations in Redash 🎉 I did a quick review of your code and left few comments. In general it looks quite good, I'll do a deeper review a bit later.

<div className="form-group">
<label className="d-flex flex-column m-b-10" htmlFor="pivot-show-controls">
<span className="m-r-10">Ok Range</span>
<input className="form-control" type="text" value={options.okRange} onChange={event => updateOptions({ okRange: event.target.value })} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please try to use components from Ant library (in this case, import Input from 'antd/lib/input' and remove className attribute - it will not be needed). Also, for colors we offer user a dropdown with list of predefined colors (for consistency - import ColorPalette from '@/visualizations/ColorPalette'), and add a custom color option there if needed.

Copy link
Author

Choose a reason for hiding this comment

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

image
changing className to class causes errors, see attachment

regarding the colorPallet I would be happy to replace but I did not understand how erectly to use it , can you explain how it is used

Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace

<input className="form-control" type="text" value={options.okRange} onChange={event => updateOptions({ okRange: event.target.value })} />

with

import Input from 'antd/lib/input';

<Input value={options.okRange} onChange={event => updateOptions({ okRange: event.target.value })} />

The same for other text inputs.

For color inputs I'll prepare an example a little bit later.

client/app/visualizations/gauge/Editor.jsx Outdated Show resolved Hide resolved
client/app/visualizations/gauge/gauge.less Outdated Show resolved Hide resolved
client/app/visualizations/gauge/index.js Outdated Show resolved Hide resolved
client/app/visualizations/gauge/index.js Outdated Show resolved Hide resolved
client/app/visualizations/gauge/index.js Outdated Show resolved Hide resolved
@kravets-levko kravets-levko changed the title add gauge visualization Add gauge visualization Aug 15, 2019
@kravets-levko
Copy link
Collaborator

I realized that Plotly (library we use for a lot of other charts) already have a gauge chart: https://plot.ly/javascript/gauge-charts/

WDYT about using it instead of your custom implementation?

@deecay
Copy link
Contributor

deecay commented Aug 15, 2019

I realized that Plotly (library we use for a lot of other charts) already have a gauge chart:

I was just going to mention that.

Previous discussion. See Indicator examples.
Is this worth the increased dependency?

@kravets-levko kravets-levko changed the base branch from master to feature/add-gauge-visualization September 2, 2019 17:45
@kravets-levko kravets-levko changed the base branch from feature/add-gauge-visualization to master September 2, 2019 17:47
@kravets-levko
Copy link
Collaborator

Hey @IdoPeles,

I converted renderer component to React as well - you can pull latest changes I see how I did it. But your repo is quite out of date with ours, for example resizeObserver service is missing (but it's needed to replace a resize-event directive). So I kindly ask you to catch up with our repo, and then we can continue with this visualization.

@arikfr
Copy link
Member

arikfr commented Jan 21, 2020

Seems like this is abandoned. I'm closing this now, but you're welcome to re-open after addressing the comments. Thanks!

@arikfr arikfr closed this Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants