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.

Sign up for GitHub

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

Jump to bottom

Allow user to specify resolution when downloading a screenshot #339

Open
keyserj opened this issue Feb 19, 2024 · 35 comments · May be fixed by #530
Open

Allow user to specify resolution when downloading a screenshot #339

keyserj opened this issue Feb 19, 2024 · 35 comments · May be fixed by #530
Assignees
@Emmanuel-222
Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest good ticket for hacktoberfest QoL small change the improves the feel of using the tool
Milestone
update #X?: worth...

Comments

@keyserj
Copy link
Collaborator

keyserj commented Feb 19, 2024 •
edited
Loading

Is your feature request related to a problem? Please describe.
I'm happy that I can take a high quality image of my diagram via the screenshot button, but I want to be able to increase or decrease quality and change the dimensions as I want, so that I can get exactly the image I want, perhaps for a desktop background, or a print-out diagram.

Describe the solution you'd like
When I click the screenshot button, I want to specify the resolution of the image:
image

Describe alternatives you've considered

Additional context
The screenshot button is in the MoreActionsDrawer, accessed here:
image

Technical ideas

  • MUI's dialog and text field (with input adornments) components seem like they should work well for this
  • Zod + react-hook-form could also be good for ensuring the inputs are valid when submitting the dialog, similar to how we validate topic description here:
    image
@keyserj keyserj added enhancement New feature or request needs review Hasn't been reviewed by a maintainer yet needs refining More details should be discussed before implementing labels Feb 19, 2024
@keyserj keyserj changed the title Allow user to specify resolution when downloading a screenshot of their diagram Allow user to specify resolution when downloading a screenshot Feb 19, 2024
@keyserj keyserj added good first issue Good for newcomers QoL small change the improves the feel of using the tool and removed needs review Hasn't been reviewed by a maintainer yet needs refining More details should be discussed before implementing labels Feb 20, 2024
@keyserj keyserj added the hacktoberfest good ticket for hacktoberfest label Sep 17, 2024
@Emmanuel-222
Copy link

Please assign this to me.

@Emmanuel-222
Copy link

Thank you so much

@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@Emmanuel-222
Copy link

Emmanuel-222 commented Oct 7, 2024 via email

@keyserj
Copy link
Collaborator Author

keyserj commented Oct 7, 2024

Hey @Emmanuel-222 no problem 🙂

  1. check out the CONTRIBUTING.md file; it's got info about running the project, an overview of the code, and a link at the bottom for the PR process (you'll need to fork this repo and make your changes on your own branch there)
  2. I realize the text in the screenshot is a bit outdated, it says "download screenshot" now - if you search for that text in the repo you'll find the current logic for downloading a screenshot without yet being able to specify the resolution
  3. instead of downloading the screenshot right away, we should be able to open a modal to specify the resolution (then confirming the dialog should trigger the download logic). we're actually doing something similar for renaming a View, so that logic might be able to help you get started. you can find that View-rename dialog on the playground:
    image

How does that sound? Let me know if you have any more questions.

@Emmanuel-222
Copy link

Emmanuel-222 commented Oct 7, 2024 via email

@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222

This comment was marked as resolved.

Sign in to view
@keyserj

This comment was marked as resolved.

Sign in to view
@Emmanuel-222
Copy link

Screen.Recording.2024-10-10.141704.mp4

This is what I have done so far. What do you think?

@keyserj
Copy link
Collaborator Author

keyserj commented Oct 10, 2024

@Emmanuel-222 functionality seems good, but I'm hoping for something like this:
image

  • defaulting the resolution to 2560 x 1440 because sometimes users won't care and just want something reasonable
  • both values visible at the same time
  • explicit adornment "px" so the user knows the units
  • validation that a number is entered
  • matching the style of the rest of the app

I'm also not sure if specifying a quality is necessary - I was envisioning the pixel resolution to indicate both the size and quality of the image.

You should be able to find good guidance for this work by referring to the Technical ideas section of this issue as well as the logic around the "Edit View" button, as that opens to a modal with validation, that should be similar to what you need to do here.

chrome_2024-10-10_10-13-35

@Emmanuel-222
Copy link

Hi @keyserj,

This is what I have worked on so far. What do you think?

Screen.Recording.2024-10-14.153246.mp4

@keyserj
Copy link
Collaborator Author

keyserj commented Oct 14, 2024 •
edited
Loading

Hey @Emmanuel-222, that looks pretty cool! I use a tool to take screenshots like this all the time, and it's nice to see what the code might look like.

But I think for the purposes of this issue, this solution has a few significant drawbacks.

First off, to clarify: the width and height that we're currently using for the resolution do not affect what content is in the image, they only affect the size that the image is scaled to. The nodeBounds variable is what determines what's in the image.

So the area being captured for the screenshot is currently unchangeable - it always captures all of the nodes being displayed, fits them within the image, and centers them. That's intentional, so that you don't have to spend effort repositioning or zooming-to-fit your diagram, or only snapping certain parts of it. If you ever want to only capture certain nodes, you can just filter out the other nodes via the app's filtering functionality. Manually capturing an area takes more work to get what you want, and each snapshot you take will likely be slightly different than if you try taking another.

Another downside of the manual capture is that you can only capture a region that fits within your monitor's size. The default resolution is currently larger than my monitor, which enables a higher quality e.g. you can zoom into the screenshot and see more pixels than you can see just looking at the diagram via the app on your monitor.

For these reasons, I think the solution initially suggested by this issue would be better. Namely, this is the solution in the image below, where a modal is provided to enter pixel values if the user desires.
image

@Emmanuel-222
Copy link

Can you break this down?, I really don' understand it.

@keyserj
Copy link
Collaborator Author

keyserj commented Oct 15, 2024

@Emmanuel-222

Regarding how the code currently works: if you change the lines that hardcode the resolution, say you decrease these from 2560 x 1440 to 1920 x 1080, you'll still get the same nodes in the image, centered in the same spot. But the KB of the image file will be smaller, and the image might look a little more blurry, because there are fewer pixels being used.

This GitHub issue is asking for a modal where you can enter these two numbers. This modal should look and behave very similarly to the Quick View modal in the gif below (but instead of accepting Title, it should accept Width and Height:
chrome_2024-10-10_10-13-35

Let me know if there's anything more specific that I can clear up.

@Emmanuel-222
Copy link

Emmanuel-222 commented Oct 16, 2024 via email

@keyserj
Copy link
Collaborator Author

keyserj commented Oct 17, 2024

@Emmanuel-222 as I mentioned above, dragging the cursor to take the screenshot is not desirable because:

  • user has to spend effort repositioning the diagram before screenshotting. with the current solution, the diagram is automatically centered in the screenshot.
  • user has to spend effort setting the desired zoom before screenshotting. with the current solution, the diagram is automatically zoomed to fit the size of the screenshot.
  • user can only capture a screenshot that fits within their monitor's size. with the desired solution (entering width x height), you can take a screenshot of any resolution quality you want.

Do these points make sense?

@Emmanuel-222

This comment was marked as off-topic.

Sign in to view
@keyserj

This comment was marked as off-topic.

Sign in to view
@keyserj
Copy link
Collaborator Author

keyserj commented Dec 27, 2024

Hey @Emmanuel-222 thanks for looking at this ticket - I'm going to unassign you to open this ticket for others. Let me know if you're still working on it and I can reassign you.

@Emmanuel-222
Copy link

Emmanuel-222 commented Dec 27, 2024 via email

@keyserj keyserj linked a pull request Dec 27, 2024 that will close this issue
docs(contributing): add non-code section #530
Open
@Emmanuel-222
Copy link

Emmanuel-222 commented Dec 27, 2024 via email

@keyserj
Copy link
Collaborator Author

keyserj commented Dec 27, 2024

Once on this issue, once on the associated PR.

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

@Emmanuel-222 Emmanuel-222

Labels
enhancement New feature or request good first issue Good for newcomers hacktoberfest good ticket for hacktoberfest QoL small change the improves the feel of using the tool
Projects
ameliorate
Status: No status
Milestone
update #X?: worth doing at some point
Development

Successfully merging a pull request may close this issue.

docs(contributing): add non-code section Emmanuel-222/ameliorate
2 participants
@keyserj @Emmanuel-222