Skip to content
This repository has been archived by the owner on Dec 7, 2021. It is now read-only.

fix: Tagging new region only applies to one region [AB#17176] #635

Merged
merged 10 commits into from
Mar 6, 2019

Conversation

tbarlow12
Copy link
Contributor

  • Canvas Helpers now returns clones of transformed tag arrays
  • Canvas now allows CanvasTools to manage selected regions (removed from Canvas state)
  • Fixing tag tests from editor page

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #635 into dev will increase coverage by 0.25%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #635      +/-   ##
==========================================
+ Coverage   86.46%   86.71%   +0.25%     
==========================================
  Files         114      114              
  Lines        3523     3515       -8     
  Branches      612      607       -5     
==========================================
+ Hits         3046     3048       +2     
+ Misses        477      467      -10
Impacted Files Coverage Δ
...c/react/components/pages/editorPage/editorPage.tsx 92.45% <ø> (+4.95%) ⬆️
src/react/components/pages/editorPage/canvas.tsx 84.12% <100%> (+0.79%) ⬆️
...react/components/pages/editorPage/canvasHelpers.ts 100% <100%> (ø) ⬆️

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 9daacad...9e2b1e2. Read the comment docs.

@tbarlow12 tbarlow12 requested review from mydiemho and PIC123 and removed request for mydiemho March 5, 2019 16:46
@tbarlow12 tbarlow12 changed the title WIP - fix: Tagging new region only applies to one region [AB#17176] fix: Tagging new region only applies to one region [AB#17176] Mar 5, 2019
@tbarlow12 tbarlow12 changed the base branch from v2 to dev March 5, 2019 18:19
Copy link
Contributor

@PIC123 PIC123 left a comment

Choose a reason for hiding this comment

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

Looks good!

@luisamiranda luisamiranda self-requested a review March 5, 2019 21:25
},
));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you getting rid of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I moved it somewhere else... I'll look into it

@@ -498,6 +501,27 @@ describe("Editor Page Component", () => {
dispatchKeyEvent("Ctrl+e");
expect(clearRegions).toBeCalled();
});
Copy link
Collaborator

@elizabethhalper elizabethhalper Mar 5, 2019

Choose a reason for hiding this comment

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

Make sure you are up to date with the current dev branch. The Ctrl+e is related to the export provider and I don't know if this test is valid anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll make the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I'll make the change

Copy link
Collaborator

@elizabethhalper elizabethhalper left a comment

Choose a reason for hiding this comment

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

Left a couple comments but otherwise looks good!

Copy link
Member

@luisamiranda luisamiranda left a comment

Choose a reason for hiding this comment

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

All looks good, but would add a note to the squash/merge commit about fixing the originally named bug too.

@tbarlow12 tbarlow12 merged commit 300d491 into dev Mar 6, 2019
wbreza pushed a commit that referenced this pull request Mar 9, 2019
Originally meant to fix a bug for hot keys applying the wrong tag to a region, but then discovered that the root cause was that each tag was being toggled on every region. If multiple tags were applied to multiple regions, it appeared that the wrong tag was being applied.

Resolves [AB#17176]
@mydiemho mydiemho deleted the tabarlow/tag-keyboard-fix branch March 20, 2019 21:25
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.

4 participants