feat(region): Hide annotations if visibility is false#483
feat(region): Hide annotations if visibility is false#483mergify[bot] merged 3 commits intobox:masterfrom
Conversation
mingzexiao6
force-pushed
the
fix-visibility
branch
from
33bbf0d to
688b8b2
Compare
mingzexiao6
force-pushed
the
fix-visibility
branch
from
1e8348d to
36a1dfc
Compare
mingzexiao6
force-pushed
the
fix-visibility
branch
from
36a1dfc to
dde6080
Compare
| if (!this.rootEl) { | ||
| return; | ||
| } | ||
| if (visibility) { |
There was a problem hiding this comment.
Should this be handled by the parent at this point?
There was a problem hiding this comment.
It seems like we're using redux to store the state, but not relying on it to hide and show the annotations, but rather rely on the value passed into setVisibility. What are your thoughts on having RegionContainer connect to this state and have RegionAnnotations use that as a basis to render RegionList?
There was a problem hiding this comment.
It seems like we're using redux to store the state, but not relying on it to hide and show the annotations, but rather rely on the value passed into
setVisibility. What are your thoughts on havingRegionContainerconnect to this state and haveRegionAnnotationsuse that as a basis to renderRegionList?
Jared thinks this logic should live in the higher level. Otherwise we'll have to replicate this logic for all future annotation types.
| if (!this.rootEl) { | ||
| return; | ||
| } | ||
| if (visibility) { |
There was a problem hiding this comment.
It seems like we're using redux to store the state, but not relying on it to hide and show the annotations, but rather rely on the value passed into setVisibility. What are your thoughts on having RegionContainer connect to this state and have RegionAnnotations use that as a basis to render RegionList?
| @@ -120,7 +120,14 @@ export default class BaseAnnotator { | |||
| }; | |||
|
|
|||
| setVisibility = (visibility: boolean): void => { | |||
| describe('setVisibility()', () => { | ||
| test.each([true, false])('should dispatch setVisibilityAction with visibility %p', visibility => { | ||
| test.each([true, false])('should hide/show annotations if visibility is %p', visibility => { | ||
| annotator.rootEl = defaults.container; |
There was a problem hiding this comment.
Probably just call annotator.init(1) to set up the annotator properly.
There was a problem hiding this comment.
Probably just call
annotator.init(1)to set up the annotator properly.
If call init but not set rootEl explicitly, eslint will throw error annotator.rootEl is possibly null
4 participants
Uh oh!
There was an error while loading. Please reload this page.