Skip to content
This repository has been archived by the owner on Jun 10, 2024. It is now read-only.

Issue#145 Add preview modal window for screenshots #191

Merged
merged 5 commits into from
Jun 15, 2023
Merged

Issue#145 Add preview modal window for screenshots #191

merged 5 commits into from
Jun 15, 2023

Conversation

ihordubas1
Copy link

@ihordubas1 ihordubas1 requested a review from kfarr June 7, 2023 17:26
@netlify
Copy link

netlify bot commented Jun 7, 2023

Deploy Preview for 3dstreet-editor-builds ready!

Name Link
🔨 Latest commit dae45b5
🔍 Latest deploy log https://app.netlify.com/sites/3dstreet-editor-builds/deploys/648b24e058c1910008fddd74
😎 Deploy Preview https://deploy-preview-191--3dstreet-editor-builds.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@kfarr
Copy link
Contributor

kfarr commented Jun 7, 2023

@ihordubas1 thanks for this! On first glance it is great progress. I can share more feedback later today.

@kfarr
Copy link
Contributor

kfarr commented Jun 8, 2023

Hi @ihordubas1 here is some feedback, let me know if any visuals are needed:

  • there is still a border when the window is closed (pressing x on the window)
  • the image is a little bit too large for the area in the dialog box (it overlaps with the png and jpeg buttons)
  • the loader isn't needed anymore (the background is black for a moment and the loader is underneath the dialog, no need for that anymore)

@ihordubas1
Copy link
Author

Hi @ihordubas1 here is some feedback, let me know if any visuals are needed:

  • there is still a border when the window is closed (pressing x on the window)
  • the image is a little bit too large for the area in the dialog box (it overlaps with the png and jpeg buttons)
  • the loader isn't needed anymore (the background is black for a moment and the loader is underneath the dialog, no need for that anymore)

thanks for comments @kfarr
updated!

@kfarr
Copy link
Contributor

kfarr commented Jun 8, 2023

thanks @ihordubas1 2 of the 3 issues were resolved, thank you!

The remaining issues are placement and sizing of the screenshot on the preview window

2 issues:

  • The placement is not correct -- it is below the dialog now and should instaed be centered in the dialog below the Save As (jpg) (png) buttons
  • The aspect ratio is incorrect -- the image is slightly decreased height ratio relative to width giving the appearance of slightly "squished" vehicles. Instead, the original aspect ratio should be maintained when resizing -- in other words whatever % the size is reduced should be identically reduced on both width and height.

See preview:
image

@kfarr
Copy link
Contributor

kfarr commented Jun 8, 2023

@ihordubas1 another issue is the sizing of the dialog box

The intent is that the dialog reflects the aspect ratio of the document window.

For example, the dialog could be approx 80% of height and width of the viewport and the image within the dialog could be 70% of the height and width. (That's just an example, the exact numbers may be a bit different.)

@kfarr
Copy link
Contributor

kfarr commented Jun 8, 2023

Ok I fixed the width and height of the dialog box with this commit 4b1c7f0

Next is to ensure that the image is placed and sized correctly.

I notice that you are still using this sample code, this was intended as an example, not to literally use as is:
'';

For example, instead of positioning this way the img could be a child of the dialog box node and positioned using vw and vh such as 80vh 80vw

Does that make sense?

@kfarr
Copy link
Contributor

kfarr commented Jun 15, 2023

this looks great, thanks @rostyslavnahornyi

image

@kfarr kfarr merged commit bb3c456 into master Jun 15, 2023
@kfarr kfarr deleted the issue#145 branch June 15, 2023 19:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants