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

plots autoresize; plus other fixes #39

Closed
wants to merge 3 commits into from

Conversation

timotheecour
Copy link
Contributor

@timotheecour timotheecour commented Jan 26, 2019

/cc @Vindaar @brentp
caveat: this will ignore given size settings; auto resize is a better default than a fixed arbitrary size, but we shd honor size if provided by user; let me know if u have a quick way to patch this PR to do that

Copy link
Member

@Vindaar Vindaar left a comment

Choose a reason for hiding this comment

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

Thanks for this! Aside from a few minor, it's great.

I agree, removing the file is unnecessary. Especially if we overwrite a file anyways. What do you think, @brentp? It's probably a good idea though to properly fix #20 soon though (I'll comment over there further).

removeFile should also be taken out for compilation with --threads:on, if we change it.

The resizing is really nice. I didn't know I wanted it until now. :) However, I'm not sure if we should default to this behavior. See the comment.

};
window.onresize = runRelayout;
# Consider: {responsive: true}
Plotly.newPlot('plot0', $data, $layout).then(runRelayout);
Copy link
Member

Choose a reason for hiding this comment

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

I would remove the <script> body from the defaultTmplString template, and create two different bodies. One with your JS code and one with the old one. Then the comments can just be normal Nim comments above the definition of that code and we can avoid the removeComments iteration.

Then add a field to Layout here: https://github.com/brentp/nim-plotly/blob/master/src/plotly/plotly_types.nim#L249. Maybe autoResize or something. Depending on its value either take your JS code or the old one. Then we can decide what we want to default to.

Copy link
Collaborator

@brentp brentp left a comment

Choose a reason for hiding this comment

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

These look good to me. Except, I don't really like nim comments in the javascript.

I think you might just need to set the plot div to have a %, like:

 <div id="plota" style="width: 100%; height: 100%"></div>

for the resize to work.

@timotheecour
Copy link
Contributor Author

I think you might just need to set the plot div to have a %, like:

first thing I tried, but it didn't work; if you find a simpler way let me know :)
will update shortly to address remaining comments

brentp pushed a commit that referenced this pull request Nov 1, 2021
* plots autoresize; plus other fixes

* clean up some imports

* [tests] fix API test due to chroma changes

* split HTML template into resizable / non-resizable part

* make auto resize user selectable, change HTML file location

- allows the user to choose whether the plots should be automatically
resized
- allows the user to choose whether to remove the temporary file
- cleans up the name for the arguments (i.e. override for temp file
location & HTML template)
- some doc string improvements

* fix filling data into HTML template, remove nim comment

* replace temp dir logic by single dir + multiple files

Before it was many dirs with a single file each. This makes more sense
and isn't as messy.

* respect autoResize

* update ubuntu image to make choosenim work

Co-authored-by: Timothee Cour <[email protected]>
@Vindaar
Copy link
Member

Vindaar commented Nov 1, 2021

Closing in favor of #72.

@Vindaar Vindaar closed this Nov 1, 2021
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.

feature: resizing plot
3 participants