Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Viewport #24

Merged
merged 21 commits into from
May 27, 2019
Merged

Viewport #24

merged 21 commits into from
May 27, 2019

Conversation

biharck
Copy link
Contributor

@biharck biharck commented May 16, 2019

Adding the ability to use hotkeys with:

Default Tool
Zoom
Pan
Angle measurement
Scroll stack
Length measurement
Flip Horizontally
Flip Vertically
Rotate Right
Rotate Left
Invert
Zoom In
Zoom Out
Zoom to Fit
Reset
Clear Tools
Scroll Down
Scroll Up
Scroll to the Last Image
Scroll to First Image

@dannyrb
Copy link
Member

dannyrb commented May 17, 2019

@biharck, can you remove the package-lock.json and add it to your .gitignore?

The main OHIF projects use yarn and commit yarn.lock


_dispatchCommand(options) {
const { setActiveViewportSpecificData } = actions
window.store.dispatch(
Copy link
Member

Choose a reason for hiding this comment

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

Is this the best/only way to access the store?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I am not 100% sure about it either. We can catch up in order to design the best approach.

Copy link
Member

Choose a reason for hiding this comment

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

@biharck we should never use window.store in this package.

We should get a callback and trigger that callback, so let the caller decide what to do with it. See how Hanging Protocol Engine do that.

@dannyrb
Copy link
Member

dannyrb commented May 17, 2019

You added a new Action, which broke a unit test. Can you update the unit test to include the new Action? You can see the error by clicking "Details" next to the integration check.

image

It should take you here:

https://circleci.com/gh/OHIF/ohif-core/70?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

@dannyrb dannyrb self-requested a review May 17, 2019 16:25
@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #24 into master will decrease coverage by 0.07%.
The diff coverage is 8.88%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #24      +/-   ##
=========================================
- Coverage    7.29%   7.22%   -0.08%     
=========================================
  Files         167     167              
  Lines        4480    4538      +58     
  Branches      909     913       +4     
=========================================
+ Hits          327     328       +1     
- Misses       3271    3323      +52     
- Partials      882     887       +5
Impacted Files Coverage Δ
src/index.js 100% <ø> (ø) ⬆️
src/classes/HotkeysUtil.js 0% <0%> (-7.5%) ⬇️
src/redux/reducers/servers.js 100% <100%> (ø) ⬆️
src/redux/constants/ActionTypes.js 100% <100%> (ø) ⬆️
src/redux/actions.js 100% <100%> (ø) ⬆️
src/redux/reducers/viewports.js 70% <37.5%> (-30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8e57cc...21df3ae. Read the comment docs.

@dannyrb
Copy link
Member

dannyrb commented May 20, 2019

@biharck if you resolve @evren217's issue and the conflict, then I think we can merge this and your other PR. I do believe we will want to change these actions in the (very) near future to use the exposed cornerstone DOM element instead of reacting to state changes though.

@biharck biharck merged commit 7832709 into master May 27, 2019
@biharck biharck deleted the viewport branch May 27, 2019 11:21
@evren217 evren217 restored the viewport branch May 27, 2019 14:51
evren217 added a commit that referenced this pull request May 27, 2019
This reverts commit 7832709.
@evren217 evren217 mentioned this pull request May 27, 2019
evren217 added a commit that referenced this pull request May 27, 2019
@evren217 evren217 deleted the viewport branch May 27, 2019 15:00
@ohif-bot
Copy link
Member

🎉 This PR is included in version 0.5.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants