Add position reset button and update zoom interaction. (#292) (#345) #372

alisaitbilgi · 2018-12-12T16:26:35Z

There are two things that I am not sure about. First one is reset button's icon. I left it as "reset" but i think, it is better to replace it with a meaningful icon :) and secondly, I just thought that for each zoom interaction triggered by zoom-in / zoom-out buttons, we should center the image in order not to loose it. When image is close to corners, if zoom-in / zoom-out buttons are pressed, it is getting disappeared. Other than those, I hope I could be useful :)

googlebot · 2018-12-12T16:26:40Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

alisaitbilgi · 2018-12-12T16:34:29Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).

The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.

The email used to register you as an authorized contributor must also be attached to your GitHub account.

I signed it!

googlebot · 2018-12-12T16:34:32Z

CLAs look good, thanks!

jakearchibald · 2018-12-13T09:14:34Z

@kosamari @mustafa-x @developit @surma what do you think about this?

Clicking zoom buttons centers image. I think we debated this at some point, but I think it might make sense, especially if the image at the destination zoom can be contained within the view. Maybe not when it covers the view.

The zoom seems to have a minimum of 25% which seems too high. Also, it feels like the steps should be more exponential.

Reset button. I expected it to also reset zoom to 100%. I'm not sure if this is useful.

I haven't really looked at the code yet, so I'm just looking at the interactions.

alisaitbilgi · 2018-12-13T10:10:05Z

@jakearchibald thanks a lot for the initial reviews,
Reset button: It just resets the position as mentioned at #292 . On the other hand, when you double-click, image is reset to 100% zoom and center position as mentioned at #345 . But, I think two issues can be somehow merged and since zoom-in/out buttons now resets the position (if you think so), it may become redundant to have as a special button.

jakearchibald · 2019-02-04T15:03:00Z

Ping @kosamari @mustafa-x @developit @surma. Curious to know your thoughts on this feature.

Preview: https://deploy-preview-372--squoosh.netlify.com/

surma · 2019-02-04T15:39:46Z

Thanks @alisaitbilgi for working on this!

Here’s my feedback:

I agree with Jake that the reset functionality should also reset zoom level to 100%
I feel weird about the +/- buttons resetting the position. My gut feeling is that a common workflow is to center the ROI and then zoom in and out, trying different codecs. That would get really annoying with this change.
Locking to 25% steps seems quite big at the smaller zoom levels and very small for the higher zoom levels. The difference from 25% to 50% is massive, while the difference from 625% to 650% is almost negligible. I think exponential steps are preferable.
I am a bit hesitant on the button as screen estate is scarce, especially on mobile. As you mentioned, we originally considered to reset zoom and position when you double-click the zoom value. Do you wanna do that instead?

mustafa-x · 2019-02-04T16:13:59Z

At a glance, I thought the reset button would reset everything to its initial loaded state. Also the text feels a tad too small. Zooming should be consistent, the snapping to the centre looks a bit broken.

The rotate buttons has a weird glitch where it warps the image before rotation.

jakearchibald · 2019-02-04T16:43:11Z

The rotate glitch is unrelated to this PR. I think that happened once we started debouncing encoding.

alisaitbilgi · 2019-02-04T17:04:45Z

@jakearchibald , @surma and @mustafa-x thanks a lot for the reviews. Here are my ideas:

Reset button: I think I can remove that button and instead, extend the behaviour of double click as making it reset the image to its initial *loaded state.
Zoom level steps: It can stay as it is. (Already exponential)
Resetting position: I think to overcome the loosing the image issue when zoom in/out, instead of resetting its position on every click, I can observe the intersection within the viewport and then, reset the position when it is not contained in the view.

What do you think?

surma · 2019-02-04T17:06:16Z

I can observe the intersection within the viewport and then, reset the position when it is not contained in the view.

That sounds reasonable! I’d have to actually play with it to see if it makes sense, but it sounds good on paper!

alisaitbilgi · 2019-02-04T17:08:50Z

I can observe the intersection within the viewport and then, reset the position when it is not contained in the view.

That sounds reasonable! I’d have to actually play with it to see if it makes sense, but it sounds good on paper!

Thanks a lot @surma , I'm planning to use Intersection Observer API but, haven't decided the threshold value yet, I think it is better to see then decide.

alisaitbilgi · 2019-02-04T20:26:27Z

Here is the implementation @surma , @jakearchibald , @mustafa-x. The thing that I'm not sure about the intersection observation behavior is that, I'm not tracking the next position after zoom out/in buttons are triggered. Actually after target element has lost its visibility 50%, next zoom out/in button interaction will cause the image to reset its position.

alisaitbilgi · 2019-02-04T20:58:59Z

By the way, sorry about forgotten intersection observer polyfill. I don't know the right way is whether to install polyfill or just feature detect the API?

src/components/Output/index.tsx

surma · 2019-02-06T15:58:42Z

Good work!

Couple of things:

Yes, definitely lazy-load a IO polyfill :)
I don’t think we want to reset on double-click anywhere. It’s a common gesture to zoom on double-tap on native apps. While we don’t necessarily need to implement that gesture, we definitely shouldn’t change the meaning of the gesture. I’d say we put the double click gesture onto the percentage.
It’s still fairly easy to lose the image if you put the image in one corner and your mouse in the opposite corner

alisaitbilgi · 2019-02-07T15:04:26Z

Thank you @surma :)

When I load the polyfill at first glance, I've realized an issue about threshold's being handled. I am first planning to be sure about the issue and try to fix, then apply here as I think it's an obvious one.
After some conversations I thought that with double-click we can have a reset-everything behaviour, but I'm not really sure about what to do :)
First I just controlled the image to hold it in viewport for every user interactions then I thought that, mouse is already a user controlled thing, so it might be better to leave them as they want. I think I can implement two options ( Reset position for every interactions or just for buttons ) then we can see and decide.

What do you think?

alisaitbilgi · 2019-02-08T07:14:13Z

When I load the polyfill at first glance, I've realized an issue about threshold's being handled. I am first planning to be sure about the issue and try to fix, then apply here as I think it's an obvious one.

Related issue: w3c/IntersectionObserver#345

alisaitbilgi · 2019-02-11T08:12:29Z

Here is the implementation @surma , I've just added a ( weird :) ) checkbox on top-right corner, so that we can see the difference between controlling 'wheel event' or not.

surma · 2019-02-18T23:29:36Z

Good work on the lazyloading. That looks good to me.
The zooming with the checkbox enabled is definitely worse because I can’t zoom into the top right corner of the image. I think what I showed in the GIF is a corner case, so we can ignore that one. The behavior when the checkbox is disabled seems good :)
I think double-click on the percentage field to reset everything makes more sense than double-click anywhere.

googlebot · 2019-02-19T16:34:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

Googlers can find more info about SignCLA and this PR by following this link.

googlebot · 2019-02-19T18:03:13Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Googlers can find more info about SignCLA and this PR by following this link.

alisaitbilgi · 2019-02-19T18:13:23Z

Hey @surma , I think I made things more complicated while fixing my 'commit author' mistake. :) I was using my brother's computer and his name has appeared as commit author for my 2 commits. (1dceeae and 868e8bb I've tried to update them but since one of them (1dceeae) is a merge commit, the situation got complicated. What do you think?

surma · 2019-02-25T09:43:33Z

Uh, this looks a bit messy, doesn’t it 😂

I’d say we just squash them and go from there. You comfortable doing that? Otherwise, with your permission, I can do it for you.

alisaitbilgi · 2019-02-25T09:55:40Z

Thanks a lot for the help :) , do you mean to squash my latest (messy) commits?

surma · 2019-02-25T10:19:05Z

Yeah, basically turn this PR into a single commit based on current master, and then we can go from there :)

alisaitbilgi · 2019-02-25T16:01:35Z

Sorry for late response, @surma. I am not sure about squashing merge commits and re-writing their history. I hope, didn't make things much more complicated.

surma · 2019-02-25T19:33:54Z

@alisaitbilgi I fixed it for you :)

Please make sure you reset your local branch appropriately.

In case you don’t know how that works, here is a step-by-step:

$ git fetch --all
$ git checkout update-zoom-interaction
$ git reset --hard origin/update-zoom-interaction

(You might have to replace origin with whatever you called your fork)

alisaitbilgi · 2019-02-26T15:22:18Z

Thanks a lot and done :) What about the PR then?

surma · 2019-02-26T15:24:06Z

Right! So this looks good to me!

Imma hand the final LGTM to @jakearchibald for a sanity check. Thanks so much @alisaitbilgi

jakearchibald · 2019-02-26T15:59:57Z

Can someone write a summary of what the intended changes are from a user perspective?

Right now, if I open the image of the red panda, position its eye in the centre, and start pressing +, at some point the image jumps so I'm no longer zooming into the eye. It isn't clear if this is an error or intended behaviour.

Also, if I double click the % value, I get "Processing error". I'm assuming that isn't intended behaviour 😄.

alisaitbilgi · 2019-02-26T16:13:41Z

Can someone write a summary of what the intended changes are from a user perspective?

We decided to make 2 things:

Add reset everything behavior when % value is double clicked.
Add reset position behavior when image is lost from the viewport (threshold: 0.5) if it's being lost by clicking zoom in/out buttons.

Also, if I double click the % value, I get "Processing error". I'm assuming that isn't intended behaviour 😄.

Yeah, I was looking at that error. I was using '360' for rotation value and now updating as '0'. (this will fix) On the other hand, found one more error for development mode and investigating to fix it. HMR cannot dynamically import 'intersection-observer-polyfill' and throws an error.

alisaitbilgi · 2019-03-04T13:09:55Z

Here are the updates @surma.

Rotation issue is fixed.
There was an HMR issue. Since dynamically importing 'intersection-observer' fails (because of HMR), next lines are re-run while prev-lines becomes unchanged and not compiled/run( I have no idea why ) an error was thrown. I've fixed this dev-only issue by only swapping 2 checks, but it may be done something like that:

if (!('intersectionObserver' in window)) {
      if (process.env.NODE_ENV === 'development') {
        require('intersection-observer');
      } else {
        await import('intersection-observer');
      }
}

I thought, writing a dev-only code is really a bad idea. So, only swap lines as a workaround.

There was a closed issue TypeError in offliner.ts #342. I think it is still alive and tried to fix it.
@jakearchibald stated "if this is an error or intended behaviour" for reset position behaviour and I've updated the intersection threshold from 0.5 to 0.2. I think this may look better.

surma

Double-clicking the percentage to reset works for me without a processing error. @jakearchibald can you confirm?
The +/- buttons are still confusing. I don’t the the position should reset when I zoom into a part of a picture. It should only reset once the image is completely invisible in my opinion.

surma · 2019-03-05T12:39:39Z

src/lib/offliner.ts

@@ -49,6 +49,8 @@ export async function offliner(showSnack: SnackBarElement['showSnackbar']) {
    navigator.serviceWorker.register('../sw');
  }

+  if (!('serviceWorker' in navigator)) return;


This should happen before the register call.

Fix set-rotation and HMR issues Fix offliner.ts dev env issue. GoogleChromeLabs#342 Reset position only if image gets totally lost from the viewport Migrate service worker existenceny check before its register call Fix re-compression issue ocurring after double click

surma requested changes Feb 6, 2019

View reviewed changes

src/components/Output/index.tsx Show resolved Hide resolved

surma force-pushed the update-zoom-interaction branch from 1c5510f to 9c81312 Compare February 25, 2019 19:32

surma approved these changes Feb 26, 2019

View reviewed changes

alisaitbilgi force-pushed the update-zoom-interaction branch from 9c81312 to 04846ae Compare March 4, 2019 12:50

surma requested changes Mar 5, 2019

View reviewed changes

alisaitbilgi force-pushed the update-zoom-interaction branch from 04846ae to a52ad38 Compare March 5, 2019 13:05

alisaitbilgi force-pushed the update-zoom-interaction branch from a52ad38 to c8ba364 Compare March 5, 2019 18:12

surma closed this Jun 15, 2020

surma reopened this Jun 16, 2020

surma changed the base branch from master to dev June 16, 2020 11:12

Add position reset button and update zoom interaction. (#292) (#345) #372

Are you sure you want to change the base?

Add position reset button and update zoom interaction. (#292) (#345) #372

Conversation

alisaitbilgi commented Dec 12, 2018

googlebot commented Dec 12, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

alisaitbilgi commented Dec 12, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Dec 12, 2018

jakearchibald commented Dec 13, 2018

alisaitbilgi commented Dec 13, 2018 • edited Loading

jakearchibald commented Feb 4, 2019

surma commented Feb 4, 2019

mustafa-x commented Feb 4, 2019

jakearchibald commented Feb 4, 2019

alisaitbilgi commented Feb 4, 2019 • edited Loading

surma commented Feb 4, 2019

alisaitbilgi commented Feb 4, 2019

alisaitbilgi commented Feb 4, 2019 • edited Loading

alisaitbilgi commented Feb 4, 2019

surma commented Feb 6, 2019

alisaitbilgi commented Feb 7, 2019

alisaitbilgi commented Feb 8, 2019

alisaitbilgi commented Feb 11, 2019

surma commented Feb 18, 2019

googlebot commented Feb 19, 2019

googlebot commented Feb 19, 2019

alisaitbilgi commented Feb 19, 2019

surma commented Feb 25, 2019

alisaitbilgi commented Feb 25, 2019 • edited Loading

surma commented Feb 25, 2019

alisaitbilgi commented Feb 25, 2019

surma commented Feb 25, 2019

alisaitbilgi commented Feb 26, 2019

surma commented Feb 26, 2019

jakearchibald commented Feb 26, 2019

alisaitbilgi commented Feb 26, 2019 • edited Loading

alisaitbilgi commented Mar 4, 2019 • edited Loading

surma left a comment

Choose a reason for hiding this comment

surma Mar 5, 2019

Choose a reason for hiding this comment

alisaitbilgi commented Dec 13, 2018 •

edited

Loading

alisaitbilgi commented Feb 4, 2019 •

edited

Loading

alisaitbilgi commented Feb 4, 2019 •

edited

Loading

alisaitbilgi commented Feb 25, 2019 •

edited

Loading

alisaitbilgi commented Feb 26, 2019 •

edited

Loading

alisaitbilgi commented Mar 4, 2019 •

edited

Loading