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 save graph as PNG to GRNsight #791

Merged
merged 5 commits into from
Aug 12, 2019
Merged

Add save graph as PNG to GRNsight #791

merged 5 commits into from
Aug 12, 2019

Conversation

mihirsamdarshi
Copy link
Collaborator

@mihirsamdarshi mihirsamdarshi commented Jul 31, 2019

Added solution for #59 to GRNsight. @dondi wanted to see if it was ok to use this library and if the implementation was ok.

Copy link
Owner

@dondi dondi left a comment

Choose a reason for hiding this comment

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

The code itself looks OK, but when I tested this locally, clicking on “Save Graph as PNG” produced two downloads of what appear to be the same image file. I also got some warnings that appear to be emanating from the saveSvgAsPng library (screenshot provided, showing both the double download and the warnings).

image

@mihirsamdarshi Please confirm whether you are seeing these two issues also, or if these might be isolated to my setup.

Also, from a design perspective, the “Save Graph as PNG” menu item strikes me as another “Export Data” choice, and thus best belongs within the “Export Data” submenu rather than as the last entry of the File menu.

Otherwise, things look good, but let’s try to clean up/investigate the above observations. Thanks!

@dondi dondi merged commit 1bbc4ce into beta Aug 12, 2019
@dondi
Copy link
Owner

dondi commented Aug 12, 2019

I've gone ahead and merged this into beta so it can be tried out on the next release. However the comments from the review still stand and ideally they should be addressed before we release as final.

I can move the menu item to “Export Data” pretty easily, but the other changes will take deeper investigation (and confirmation first to see if it happens on other setups).

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.

2 participants