feat(region): Hide annotations if visibility is false #483
Conversation
mxiao6
commented
May 8, 2020
•
mxiao6
commented
- Tests
| 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 => { | |||
| @@ -130,10 +130,10 @@ describe('BaseAnnotator', () => { | |||
| }); | |||
|
|
|||
| 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
