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

clippath woes #730

Closed
davisford opened this issue Oct 16, 2014 · 7 comments
Closed

clippath woes #730

davisford opened this issue Oct 16, 2014 · 7 comments
Milestone

Comments

@davisford
Copy link
Contributor

Hi, on my charts, I'm trying to render to PDF, or print, and I'm using elasticX + elasticY on some charts. It seems when I do, and if I print to PDF, the charts get clipped. I've diagnosed it down to the clippath property that gets inserted on the outer <g> element.

For example, I can use the SVG Crowbar tool to carve out an svg from my page, download it and load in a new url, and I end up with this:

Original graph:

screen shot 2014-10-16 at 2 16 00 am

After SVG Crowbar:

screen shot 2014-10-16 at 2 16 19 am

The clippath is the problem

screen shot 2014-10-16 at 2 16 36 am

Specifically, the attribute clip-path="url(#hourlysale-hour-bar-clip)"

If I manually remove it from the <g> element the problem goes away:

screen shot 2014-10-16 at 2 16 53 am

I found issue #178 which is related, but i'm still not clear on why the clippath is there as an embedded url? I guess I can manually remove this with d3.select('chart-body').attr('clip-path', ''), but not sure if this is the right way to go?

@gordonwoodhull
Copy link
Contributor

Is the clip path wrong or is the URL screwing up SVG Crowbar somehow? Or are you scaling when you export?

It is mostly there because the data does not always fit in the viewport, e.g. when zooming. It is often safe to remove it.

@davisford
Copy link
Contributor Author

@gordonwoodhull more investigation here. There are a couple of issues I've discovered. The clippath is definitely wrong, and it is wrong precisely b/c on a browser resize event (debounced), I reset the svg width to 100% and issue a chart.redraw(). This works fine but I can see that the clip path is not re-generated with a chart.redraw(), but only on chart.render().

So, I inserted it to see if that resolves it coordinate-grid-mixin.js:

    _chart._doRedraw = function () {
        _chart._preprocessData();

        drawChart(false);
        generateClipPath();
        return _chart;
    };

..and indeed, a new clippath is created, but each time it appends instead of selecting the old one, so every resize, I end up with another clippath, with the last appended child being the correct size.

A bit of experimentation in the console shows that this code just doesn't work for clippath elements:

// will *always* append a new one
var chartBodyClip = dc.utils.appendOrSelect(defs, 'clipPath').attr('id', getClipPathId());

This appears to be an old bug in WebKit browsers that still affects modern Chrome, at least.

Try d3.select('defs').selectAll('clippath') or d3.select('defs').selectAll('clipPath') -- nothing happening.

I would rather redraw my charts vs. render them, b/c it adds the nice animation as it fills out the new size of the container, but now it always leaves the clippath in the dust.

I would propose to call generateClipPath() somewhere in the path of chart.redraw(), and also need to fix it so instead of calling dc.utils.appendOrSelect(defs, 'clipPath'), select it by getClipPathId() -- if it isn't there, then append it. Does that make sense?

@gordonwoodhull
Copy link
Contributor

Okay, so this is another responsive layout bug. Unfortunately that's still not supported but it's just a matter of fixing each bug as we find it, so thanks for reporting this.

Your proposed solution sounds good. For clarity I think you could use the d3 select/append/update pattern but it's not necessary since we're talking about just one object here.

You could help others by submitting a PR if you figure this out! Sorry the updates have been slow but I'm planning to finally get 2.0 out the door after I get through another release at work.

@davisford
Copy link
Contributor Author

Cool. FWIW, the charts work surprisingly well, with only a few tweaks in a responsive environment. Perhaps it would help to just write up a wiki page on making them work in a fluid layout? I'd be happy to do that. I'll have to clean up my fork and see if I can submit a clean PR that fixes this issue.

Sometime I need to show you what we've built here. It may be the most extensive use of dc.js yet (that I've seen). It is backed by a data warehouse on AWS that is growing daily, and so far so good. Users are very happy :)

@mr23
Copy link
Contributor

mr23 commented Oct 17, 2014

Please, tell us more. Link?

@davisford
Copy link
Contributor Author

Well, unfortunately, you'll need a login, and the site is live with a lot of users right now. Here's a screenshot of one of the many reports we have. It is the backend reporting for our front-end restaurant point-of-sale system.

screen shot 2014-10-17 at 11 22 18 am

Everything on there is cross-filtered, including the table data. You can drill-down into the data 9 ways to Sunday. We've got myriad different reports, each drilling down into different parts of the data, with the backend database in a star-schema, and an ETL process that pulls data from the POS with about a 10 minute delay.

This is the same page behaving in a smaller viewport.

screen shot 2014-10-17 at 11 27 08 am

Currently, I'm working on trying to convert it to a printable PDF, which has its own set of challenges -- hence this issue (related), since charts are/were getting clipped, due to the clippath being wrong.

@davisford
Copy link
Contributor Author

@gordonwoodhull submitted a PR to fix this -- sorry for the lack of tests on some of my PRs -- they are fairly simple changes that we've been running in production for a while now if it is any consolation. I know it's not a good excuse, but I can come back when I have some additional time (which is usually NaN) and add a few more tests to these PRs.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Nov 29, 2014
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

No branches or pull requests

3 participants