refactor(regions): Split regions to separate manager#617
refactor(regions): Split regions to separate manager#617mergify[bot] merged 4 commits intobox:masterfrom
Conversation
ConradJChan
force-pushed
the
move-region-list
branch
from
2d61a7e to
eb142fe
Compare
| rootLayerEl.classList.add('ba-Layer--region'); | ||
| rootLayerEl.dataset.testid = 'ba-Layer--region'; | ||
| rootLayerEl.classList.add('ba-Layer--regionCreation'); | ||
| rootLayerEl.dataset.testid = 'ba-Layer--regionCreation'; |
There was a problem hiding this comment.
Is this going to cause issues for our automated test suites?
There was a problem hiding this comment.
Good point, I'll update the e2e tests. QA doesn't use this testid so we should be good there
| rectRef?: RegionRectRef; | ||
| }; | ||
|
|
||
| export default class RegionCreation extends React.PureComponent<Props, State> { |
There was a problem hiding this comment.
Should we convert this to a function component now?
There was a problem hiding this comment.
I'd rather do that in a follow up PR
| instance.handleAnnotationActive('123'); | ||
| (getWrapper() | ||
| .find(RegionList) | ||
| .prop('onSelect') as Function)('123'); |
There was a problem hiding this comment.
Can we avoid these casts?
There was a problem hiding this comment.
This is needed because onSelect is an optional prop
| const parentEl = referenceEl.parentNode || documentEl; | ||
|
|
||
| // Construct a layer element where we can inject a root React component | ||
| const rootLayerEl = documentEl.createElement('div'); |
There was a problem hiding this comment.
We're not seeing as much variation between managers as I expected, which is leading to a lot of duplicate code. We should consider extracting it to a base class or a helper function or something. It can wait for a future change, though.
src/image/ImageAnnotator.ts
Outdated
| if (this.managers.size === 0) { | ||
| this.managers.add(new PopupManager({ referenceEl })); | ||
| this.managers.add(new RegionManager({ referenceEl })); | ||
| this.managers.add(new RegionListManager({ referenceEl })); |
There was a problem hiding this comment.
Should the order of these be flipped?
There was a problem hiding this comment.
No, because if we do that then when the mode is region the existing regions won't receive pointer events
|
Should probably be a |
ConradJChan
changed the title
|
Mergify will use the first commit message as the final commit message. So you also need to amend the first commit message to |
ConradJChan
force-pushed
the
move-region-list
branch
from
81a70cd to
b50f4d4
Compare
3 participants
TODO