Addepar / ember-charts

https://opensource.addepar.com/ember-charts/
Other
783 stars 131 forks source link

Make separate flag for controlling the x & y axis title + Support padding for axis title #125

Closed billy-addepar closed 8 years ago

billy-addepar commented 8 years ago

This PR aims to do:

@jordansmith42 @philpee2 @embooglement

thangdinh commented 8 years ago

So this one is not backward compatible? i.e. if people are using ember-charts and this upgrade will break their existing code base where they set the hasAxisTitles. We probably should support backward compatibility here.

embooglement commented 8 years ago

I agree with Thang, if we can support backwards compatibility here we probably should, otherwise we'll have to do a major version bump.

billy-addepar commented 8 years ago

backward compatibility supported.

billy-addepar commented 8 years ago

@thangdinh @embooglement

billy-addepar commented 8 years ago

Comments addressed + tests added

thangdinh commented 8 years ago

what will happen if we have a long title?

billy-addepar commented 8 years ago

@xudaniel11work for the long title question

billy-addepar commented 8 years ago

Also @jordansmith42 for that question. It's not supported in master branch now.

jordansmith42 commented 8 years ago

@thangdinh that's an edge case that we should look into but shouldn't block this, it seems very rare that a title would be longer than the entire chart width

billy-addepar commented 8 years ago

Comments addressed

jordansmith42 commented 8 years ago

LGTM if all the comments have been addressed