Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Jul 27, 2024. It is now read-only.

Feature/dynamically set sprite sizes #119

Open
contractorwolf wants to merge 8 commits into PAIR-code:master
base: master
Choose a base branch
Loading
from contractorwolf:feature/dynamically-set-sprite-sizes
Open

Feature/dynamically set sprite sizes #119

contractorwolf wants to merge 8 commits into PAIR-code:master from contractorwolf:feature/dynamically-set-sprite-sizes

Conversation

@contractorwolf
Copy link
Contributor

@contractorwolf contractorwolf commented Apr 7, 2018

modified the approach from suggestions on #118

Copy link
Contributor Author

@jimbojw better? worse?

Copy link
Contributor Author

@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

Copy link
Contributor

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 reacted with thumbs up emoji

Copy link
Contributor

jimbojw commented Apr 16, 2018

@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.

Copy link
Contributor

jimbojw commented Apr 16, 2018

@contractorwolf got it working, looks good. Just to confirm, this only affects the demo ipynb, correct?

Copy link
Contributor

jimbojw commented Apr 17, 2018

@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?

Copy link
Contributor Author

@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
https://nbviewer.jupyter.org/github/contractorwolf/facets/blob/master/facets_dive/Dive_demo.ipynb

seems like github has some issues still with the display of notebook files :/

Copy link
Contributor Author

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

Copy link
Contributor

@jimbojw jimbojw left a comment

Choose a reason for hiding this comment

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

What is the Dive_demo.html file?

Copy link
Contributor Author

@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

Copy link
Contributor Author

@jimbojw I can do another PR without that if you want, just let me know

Copy link
Contributor

jimbojw commented Apr 27, 2018

No worries, I'm willing to merge this one, but without the HTML file. So if you can remove that, I'll push it 😎

Copy link
Contributor Author

contractorwolf commented Apr 28, 2018
edited
Loading

@jimbojw and @jameswex the new PR without the HTML file:
#123

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jimbojw jimbojw jimbojw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /