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

[Pie Chart] Add three new attributes Half circle,Pad Angle,Corner Radius to Pie Chart #1685

Closed
wants to merge 19 commits into from

Conversation

ganshanshan
Copy link

Add three new attributes Half circle,Pad Angle,Corner Radius to Pie Chart

@@ -1207,16 +1207,34 @@ class DistributionPieViz(NVD3Viz):
viz_type = "pie"
verbose_name = _("Distribution - NVD3 - Pie Chart")
is_timeseries = False
# fieldsets = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

no commented out code please

.endAngle(function(d) { return d.endAngle/2 -Math.PI/2 });
}

if (fd.pie_title){
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see references to pie_title, pie_title_pffset and pie_pad_angle anywhere else. Am i missing something?

chart.cornerRadius(fd.pie_corner_radius);
}else{
chart.cornerRadius(0);
}
break;

Copy link
Contributor

Choose a reason for hiding this comment

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

If the default is 0 you don't need the conditionals i think

"label": _("Pad Angle"),
"default": 0,
"choices": self.choicify([
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

one line should be enough for them

"label": _("Corner Radius"),
"default": 0,
"choices": self.choicify([
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

.startAngle(function(d) { return d.startAngle/2 -Math.PI/2 })
.endAngle(function(d) { return d.endAngle/2 -Math.PI/2 });
}
if (fd.pie_title){
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see references of pie_title, pie_title_offset anywhere else.

@ganshanshan
Copy link
Author

@xrmx thank you for your advice.I correct it now, you can have a check again.
Look forward to your reply.

@xrmx
Copy link
Contributor

xrmx commented Nov 24, 2016

@ganshanshan you ignored half of my comments

"label": _("Pad Angle"),
"default": 0,
"choices": self.choicify([
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

one line for all of them should be enough

"label": _("Corner Radius"),
"default": 0,
"choices": self.choicify([
0,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

return d.endAngle / 2 - Math.PI / 2;
});
}
if (fd.pie_title) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is pie_title set?

if (fd.pie_title) {
chart.title(fd.pie_title);
}
if (fd.pie_title_offset) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

if (fd.pie_title_offset) {
chart.titleOffset(fd.pie_title_offset);
}
if (fd.pie_pad_angle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if pie_pad_angle default is 0 you don't need the if

if (fd.pie_pad_angle) {
chart.padAngle(fd.pie_pad_angle);
}
if (fd.pie_corner_radius) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@ganshanshan
Copy link
Author

@xrmx Sorry for my mistake,I have update it.You can have a check.

@ganshanshan
Copy link
Author

Hi @xrmx ,if you think my code is okay,could you please merge pull reqiest?
Thanks again.

}),
'pie_half_circle': (BetterBooleanField, {
"label": _("Half circle"),
"default": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

default False

@@ -1149,6 +1149,9 @@ def visit_column(element, compiler, **kw):
qry.compile(
engine, compile_kwargs={"literal_binds": True},),
)
template_processor = get_template_processor(
Copy link
Contributor

Choose a reason for hiding this comment

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

looks unrelated

Copy link
Author

Choose a reason for hiding this comment

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

Sorry,i remove it.this is for jinja {{}} in the SQL ,no only in WHEWE or HAVING.

"label": _("Corner Radius"),
"default": 0,
"choices": self.choicify([0, 1, 2, 3, 4, 5, 6, 7, 8]),
"description": _("Pad Angle of default viewport."
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it's between 0 and 8

Copy link
Member

Choose a reason for hiding this comment

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

In [1]: range(9)
Out[1]: [0, 1, 2, 3, 4, 5, 6, 7, 8]

@@ -1208,14 +1208,21 @@ class DistributionPieViz(NVD3Viz):
verbose_name = _("Distribution - NVD3 - Pie Chart")
is_timeseries = False
fieldsets = ({
'label': None,
'label': 'Criterion',
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need the label here? Criterion does not look like the right one anyway.

Copy link
Member

Choose a reason for hiding this comment

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

agreed, better None than 'Criterion'

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, i update it.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

Would love to see screenshots, I'm curious to see whether the half-pie centers properly and how the other options look like (trying to understand how that may be preferable to existing options).

Rounding the corners may not be visually accurate as small charts get more shaved off proportionally than large ones. Not a big deal, but styling should not diminish visual accuracy.

Also to note is that you currently need to do your change in exploreV2 as well until we fully migrate there. Essentially the configuration elements in viz.py and forms.py are moving to the javascript layer. Let us know if you can't find the place to change the file and the endpoint for exploreV2 to test it

"label": _("Corner Radius"),
"default": 0,
"choices": self.choicify([0, 1, 2, 3, 4, 5, 6, 7, 8]),
"description": _("Pad Angle of default viewport."
Copy link
Member

Choose a reason for hiding this comment

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

In [1]: range(9)
Out[1]: [0, 1, 2, 3, 4, 5, 6, 7, 8]

('labels_outside', 'pie_half_circle'),
('pie_pad_angle', 'pie_corner_radius'),
),
"description": _("Properties setting of default viewport.")
Copy link
Member

Choose a reason for hiding this comment

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

_("Rendering options for the pie chart")

Copy link
Author

Choose a reason for hiding this comment

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

Thanks very much, I update my code.

Copy link
Author

Choose a reason for hiding this comment

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

To have your reply, it's my pleasure.

1.The screenshots
You can find the screenshots in #1684 .

2.I update my code.please have a check again.

3.I do not know how to change the files in exploreV2,and do not know how to test it in V2.
I issue this before #1677.

Look forward to your reply.

@@ -1208,14 +1208,21 @@ class DistributionPieViz(NVD3Viz):
verbose_name = _("Distribution - NVD3 - Pie Chart")
is_timeseries = False
fieldsets = ({
'label': None,
'label': 'Criterion',
Copy link
Member

Choose a reason for hiding this comment

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

agreed, better None than 'Criterion'

@mistercrunch
Copy link
Member

There's been merge conflicts for a long time, feel free to re-open once conflicts are sorted out.

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.

3 participants