Feature/dynamically set sprite sizes #119
Conversation
changed default sprite size to 32 to work better with large datasets
… dynamically, as well as comments to clarify
|
@jimbojw better? worse? |
|
@jameswex can I get a ruling on the new PR? I actually had some new ideas for UI changes to the deep dive display that will work better for jupyter notebook as well. If you guys take this PR (or give me some changes you want for it) I can get started on those other updates |
|
I believe @jimbojw is taking a look at this PR. As far as UI changes, our visual designer is looking at making some changes to Facets Dive. Please refrain from any visual changes to Facets Dive at this time. If you have ideas for visual changes to help with uses in notebooks, a simple explanation or mock would be better than a PR so we could discuss it with our visual designer. |
|
@contractorwolf sorry for the delay, I've been having trouble assessing the PR. The code looks fine, but I've been having trouble running it. It's probably a problem with my setup, but wanted to see it in action before signing off. |
|
@contractorwolf got it working, looks good. Just to confirm, this only affects the demo ipynb, correct? |
|
@contractorwolf you don't have to make any more changes to the ipynb file. The reason I couldn't look at the diff is because it was too large, but that's OK, I was able to run it locally. I just want to confirm that this is the only file you intended to change, right? |
|
@jimbojw yeah, I was only making the change to that one file, I cant seem to figure out why github is having trouble displaying it but I merged my changes in to my master and can show them here seems like github has some issues still with the display of notebook files :/ |
|
my changes were to figure out how to get github to display correctly, but it never did so I went back to my original code from the PR, which is what I showed before for the 119 PR |
|
@jimbojw I was trying to get the ipynb file to display correctly in github, it was the output html from that but it still doesnt display unfortunately, you can ignore that it you want |
|
@jimbojw I can do another PR without that if you want, just let me know |
|
No worries, I'm willing to merge this one, but without the HTML file. So if you can remove that, I'll push it 😎 |
modified the approach from suggestions on #118