Skip to content

Conversation

quetzaluz
Copy link
Contributor

Features in this PR:

  • Adds rightAlignYAxis option as demonstrated here:
    right-align-axis
  • Adds missing spec coverage for multiChart
  • Nests some template features in a block such that other chart types can inherit them (features now inheritable that weren't before include show_controls toggle and no_data_message

@coveralls
Copy link

coveralls commented Jan 5, 2018

Coverage Status

Coverage increased (+2.8%) to 81.67% when pulling 258c3ba on quetzaluz:feature/rightAlignYAxis-and-specs into 8c1c1fa on areski:develop.

@areski
Copy link
Owner

areski commented Jan 8, 2018

Thanks for the PR!
Should we not also add the surcharge to the other templates, for instance cumulativelinechart.html?

@quetzaluz
Copy link
Contributor Author

The other chart types also support these features (show controls toggle and no data message is universal) so I believe they should also have the template change. Otherwise they do not render these options when they are set. I can do some extra testing around this though if needed.

@areski
Copy link
Owner

areski commented Jan 8, 2018

Yes please, for sake of consistency it makes sense we update all the chart when we add an option

@quetzaluz
Copy link
Contributor Author

Looks like this will be included in cumulativelinechart.html, because that template calls super() for the body block and the new rendering_opts block is nested in body:
From nvd3\templates\cumulativelinechart.html:

{% block body %}

{# calling super guarantees everying in content is also found here ...#}
{{super()}}

{% endblock body %}

For templates that don't call super() within body (AKA they have overrides for body) I have already added this new rendering_opts block. Looks like the only chart overlooked was pie chart; I have just pushed a commit to include the change there as well.

@coveralls
Copy link

coveralls commented Jan 8, 2018

Coverage Status

Coverage increased (+2.8%) to 81.67% when pulling 554261f on quetzaluz:feature/rightAlignYAxis-and-specs into 8c1c1fa on areski:develop.

{% endif %}

{% block rendering_opts %}
{% if chart.no_data_message %}
Copy link
Contributor Author

@quetzaluz quetzaluz Jan 8, 2018

Choose a reason for hiding this comment

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

This is written out instead of just calling super because pie chart cannot have the stacked option but it can have the no_data_message and show_controls options. If we're okay with including stacked (and just count on people not trying to apply it to pie chart) I can revise this to just call super()

Copy link
Owner

Choose a reason for hiding this comment

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

good with me!

@areski areski merged commit d13a8f7 into areski:develop Jan 8, 2018
@areski
Copy link
Owner

areski commented Jan 8, 2018

Thanks for following up!

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