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

Reset rotation and position on double click #119

Merged
merged 12 commits into from
Apr 27, 2022

Conversation

harupy
Copy link
Contributor

@harupy harupy commented Apr 24, 2022

What changes are proposed in this pull request?

Reset the image rotation and position on double click.

Where does this idea come from?

This idea comes from Plotly:
https://plotly.com/python/creating-and-updating-figures/

Quick demo:

reset-on-double-click.mov

@vercel
Copy link

vercel bot commented Apr 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
image ✅ Ready (Inspect) Visit Preview Apr 27, 2022 at 7:13AM (UTC)

src/Preview.tsx Outdated
Comment on lines 232 to 237
const onDoubleClick: React.MouseEventHandler<HTMLBodyElement> = () => {
if (visible) {
setScale(1);
setPosition(initialPosition);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const onDoubleClick: React.MouseEventHandler<HTMLBodyElement> = () => {
if (visible) {
setScale(1);
setPosition(initialPosition);
}
};
const onDoubleClick: React.MouseEventHandler<HTMLBodyElement> = () => {
if (!visible) {
return;
}
setScale(1);
setPosition(initialPosition);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afc163 Thanks for the quick review! Fixed!

@harupy
Copy link
Contributor Author

harupy commented Apr 24, 2022

Is the CI / coverage check failure related to this PR? I was able to reproduce the same error on the master branch by running npm run test.

@afc163
Copy link
Member

afc163 commented Apr 24, 2022

Rebase master to fix snapshot.

@harupy
Copy link
Contributor Author

harupy commented Apr 24, 2022

Rebase master to fix snapshot.

Done!

@codecov
Copy link

codecov bot commented Apr 24, 2022

Codecov Report

Merging #119 (27a0fa9) into master (fd2d0a6) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #119   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          271       277    +6     
  Branches        81        84    +3     
=========================================
+ Hits           271       277    +6     
Impacted Files Coverage Δ
src/Preview.tsx 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

src/Preview.tsx Outdated
@@ -248,6 +256,7 @@ const Preview: React.FC<PreviewProps> = props => {
passive: false,
});
const onKeyDownListener = addEventListener(window, 'keydown', onKeyDown, false);
const onDoubleClickLister = addEventListener(window, 'dblclick', onDoubleClick, false);
Copy link
Member

Choose a reason for hiding this comment

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

useEffect return 内要卸载掉这些 addEventListener。

}
setScale(1);
setPosition(initialPosition);
};
Copy link
Member

Choose a reason for hiding this comment

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

加个 useCallBack,不然每次刷新都进下面的 useEffect 了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

完毕!

@@ -317,6 +325,7 @@ const Preview: React.FC<PreviewProps> = props => {
>
<img
onMouseDown={onMouseDown}
onDoubleClick={onDoubleClick}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afc163 What do you think about the current implementation?

src/Preview.tsx Outdated
Comment on lines 233 to 234
if (!visible) {
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
if (!visible) {
return;
if (visible) {
...
}

Can we use the initial approach to keep the test coverage 100%?

Copy link
Member

Choose a reason for hiding this comment

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

That is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, pushed a commit!

@harupy
Copy link
Contributor Author

harupy commented Apr 26, 2022

@afc163 Could you take another look?

@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

Interesting. The coverage check complained ReferenceError: mount is not defined but mount is defined.

@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

rebased on master

@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

I was wrong. mount is not defined.

@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

@afc163 Fixed tests!

Co-authored-by: afc163 <afc163@gmail.com>
@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

@afc163 Any clue on why tests failed? npm test works on my machine.

@afc163
Copy link
Member

afc163 commented Apr 27, 2022

Reinstall lock files and node_modules and try again.

@harupy
Copy link
Contributor Author

harupy commented Apr 27, 2022

I forgot to pull the latest commit.

Signed-off-by: harupy <hkawamura0130@gmail.com>
@afc163 afc163 merged commit bf1150a into react-component:master Apr 27, 2022
@harupy harupy deleted the reset-on-double-click branch April 28, 2022 02:41
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