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

Add offline support by vendoring webtreemap and using file:// URLs #26

Merged
merged 1 commit into from
Jan 19, 2016
Merged

Add offline support by vendoring webtreemap and using file:// URLs #26

merged 1 commit into from
Jan 19, 2016

Conversation

insin
Copy link
Contributor

@insin insin commented Jan 5, 2016

The vendored version of webtreemap is from this PR, which fixes a layout issue and removes the vendor prefix from CSS animation: evmar/webtreemap#2

I've also created an issue to request that webtreemap be published to npm so it doesn't have to be vendored: evmar/webtreemap#7

I'm subscribed to the PR and the issue and am willing to create further PRs to remove vendoring when possible.

Thanks for creating source-map-explorer - I found it invaluable while learning to tame Webpack bundling and continue to do so 👍

@@ -189,7 +190,9 @@ var html = fs.readFileSync(path.join(__dirname, 'tree-viz.html')).toString();

html = html.replace('INSERT TREE HERE', JSON.stringify(sizes, null, ' '))
.replace('INSERT TITLE HERE', args['<script.js>'])

.replace('underscore.js', fileURL(require.resolve('underscore')))
.replace('webtreemap.js', fileURL(require.resolve('./vendor/webtreemap.js')))
Copy link
Owner

Choose a reason for hiding this comment

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

will this break on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote it on Windows 😸 Node.js path stuff works fine on Windows if you just use / everywhere, as long as the developers of libs you're using used path.

@danvk
Copy link
Owner

danvk commented Jan 11, 2016

Thanks for the PR. It looks fine module the comments I left. It would be nice to get your PR merged into webtreemap.

@danvk
Copy link
Owner

danvk commented Jan 13, 2016

Here's a before / after for this PR on a relatively complex source map. Is this roughly the expected change? Can you summarize what's going on?

source-map-before
source-map-after

@insin
Copy link
Contributor Author

insin commented Jan 13, 2016

I assume that's due to the JavaScript tweaks in rmmh/webtreemap@318c345 - you could vendor the original instead until it's published: https://github.com/martine/webtreemap/blob/a5c3115092f3ce97c6544aff4af465a88010f01d/webtreemap.js

I only pulled in the contents from the other PR because I was playing with it locally and it had already removed the CSS vendor prefix on animation.

@danvk
Copy link
Owner

danvk commented Jan 13, 2016

I understand why the change is happening, I'm just wondering what the
high-level idea behind it is. Are the rectangles more horizontal? Laid out
differently?

On Wed, Jan 13, 2016 at 3:19 PM Jonny Buchanan notifications@github.com
wrote:

I assume that's due to the JavaScript tweaks in rmmh/webtreemap@318c345
rmmh/webtreemap@318c345

I only pulled in the contents from the other PR because I was playing with
it locally and it had already removed the CSS vendor prefix on animation.


Reply to this email directly or view it on GitHub
#26 (comment)
.

@danvk
Copy link
Owner

danvk commented Jan 19, 2016

After playing around with this, I much prefer the newer layout. I'll pull and cut a new release.

Thanks!

danvk added a commit that referenced this pull request Jan 19, 2016
Add offline support by vendoring webtreemap and using file:// URLs
@danvk danvk merged commit 707c6d3 into danvk:master Jan 19, 2016
@rmmh
Copy link

rmmh commented Jan 23, 2016

  1. it places items largest to smallest, instead of in whatever order the source data has
  2. it prefers wide rectangles to tall rectangles, since text is wide, not tall.

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