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

Don't use innerHTML on SVG elements when setting donut title #135

Merged
merged 1 commit into from
Oct 26, 2015

Conversation

spadgett
Copy link
Contributor

Fixes an issue on IE where the donut title is not set. innerHTML cannot be used reliably on SVG elements.

/cc @jwforres

@jeff-phillips-18
Copy link
Member

Running the ng-docs viewer (grunt ngdocs:view) shows that this does not work when customizing the center label HTML using the config.centerLabelFn attribute.

@spadgett
Copy link
Contributor Author

@jeff-phillips-18 Thanks, I wasn't sure how to test locally.

Any recommendation to fix? Not all browsers let you set HTML on an SVG element, so I'm not sure a custom label function that returns markup will work cross browser. I believe it's a problem on both IE and iOS.

@spadgett
Copy link
Contributor Author

Screenshot of issue on IE:

win7_ie_11 0

Not fixed by this pull request, but there is also a problem with the utilization bar chart:

bs_win7_ie_11 0

@dtaylor113
Copy link
Member

Created #136 to track the Utilization Bar Chart issue

centerLabelText.smText +
'</tspan>';
donutChartTitle.insert('tspan').text(centerLabelText.bigText).classed('donut-title-big-pf', true).attr('dy', 0).attr('x', 0);
donutChartTitle.insert('tspan').text(centerLabelText.smText).classed('donut-title-small-pf', true).attr('dy', 20).attr('x', 0);

Choose a reason for hiding this comment

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

Thoughts on having all of this in the html and then conditionally showing it via an ng-if or ng-show? To me, that would be more readable than html in JS.

Copy link
Member

Choose a reason for hiding this comment

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

The donutChartTitle html is in D3/C3

@dtaylor113
Copy link
Member

Hi, what we'd like to do is approve this change in order to get the donut chart working in IE and Safari. Next, we'll file another issue about allowing one to specify HTML in the center label.

For now, in order to get this fix in, we should update the ngdoc and specify that, at the moment, only text is supported for the centerLabelFn.

So, could you please:

  1. update donut chart's ngdoc example

    centerLabelFn - user defined function to customize the center label (optional)
    ..to...
    centerLabelFn - user defined function to customize the text of the center label (optional)

  2. update the script.js of the donut chart ngdoc from:
    $scope.custConfig = {

     'centerLabelFn': function () {
         return '<tspan dy="0" x="0" class="donut-title-big-pf">' + $scope.custData.available +   
           '</tspan>' +
                  '<tspan dy="20" x="0" class="donut-title-small-pf">Free</tspan>';
         },
    

    ...to...
    'centerLabelFn': function () {
    return $scope.custData.available + " GB";
    },

@spadgett
Copy link
Contributor Author

@dtaylor113 Thanks, I've made the suggested changes.

@dtaylor113
Copy link
Member

Hi, this needs to be rebased.
Thanks,

  • Dave

Fixes an issue on IE where the donut title is not set.
@spadgett
Copy link
Contributor Author

@dtaylor113 Rebased.

dtaylor113 added a commit that referenced this pull request Oct 26, 2015
Don't use innerHTML on SVG elements when setting donut title
@dtaylor113 dtaylor113 merged commit e66210f into patternfly:master Oct 26, 2015
@spadgett
Copy link
Contributor Author

@erundle @rhamilto We were using angular-patternfly 2.3.1 for the hot fix.

@dtaylor113
Copy link
Member

angular-patternfly v2.5.0 should be released tomorrow which will have this fix; will you be able to update to it?

@erundle
Copy link

erundle commented Oct 26, 2015

@dtaylor113 they need a patch release. I have already created a 2.3.x branch for them.

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.

5 participants