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

Fix issue with screenshot size #114

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scottwittenburg
Copy link
Collaborator

Currently just fixes the captured screenshot size at 512x512, but could be extended in the future to allow the user to specify an arbitrary size. Depends on this PR getting merged into vtk.js, then we can bump the version we depend on here.

Fixes #99

@jimklo
Copy link

jimklo commented May 12, 2021

@scottwittenburg I've pulled and tried this on our integration environment. I've tried resizing my browser and taking various screen shots, and it's still sending images larger than 512x512 px.

@scottwittenburg
Copy link
Collaborator Author

scottwittenburg commented May 12, 2021

@jimklo That's because it depends on a change in vtk.js which isn't merged yet. As I mentioned in the description, we need that to get merged and generate a new version, then we can bump our dependency on vtk.js to pick it up. I changed this to a draft.

@scottwittenburg scottwittenburg marked this pull request as draft May 12, 2021 00:30
@scottwittenburg scottwittenburg force-pushed the fix-screenshot-size-issue branch from 15ce1ff to 7911f44 Compare May 21, 2021 15:58
@scottwittenburg scottwittenburg marked this pull request as ready for review May 21, 2021 15:59
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

I guess this should now be working?

@aashish24
Copy link
Contributor

@dchiquito could you look over at this issue with help from @scottwittenburg and @dzenanz

@jimklo
Copy link

jimklo commented May 24, 2021

The screen shot issue is somewhat fixed. 512x512 max proportions seems fine. The problem we seem is that there is too much black, and the actual MRI image is very small and low resolution in comparison to the image size embedded in the email.
Image sample from 4K browser

The browser size and host resolution seems to also still play a variable, as when I took snapshots using my iPad, the scan better fills the 512x512 box, however the image resolution seems low the size.
Image Sample from iPad Pro 12.9"

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.

The size of emailed screenshots of the same image varies with browser window size
4 participants