feat(annotations): update mode if necessary for experiences #1375
Conversation
ghost
self-requested a review
as a 
| [true, false], | ||
| [false, true], | ||
| [false, false], | ||
| ].forEach(([canShow, enableAnnotationsImageDiscoverability]) => { |
There was a problem hiding this comment.
Can we use test.each for this, instead?
| const { experiences = {} } = this.options; | ||
| const canShow = Object.values(experiences).some(experience => experience.canShow); | ||
|
|
||
| return !canShow && !!this.options[discoverabilityType]; |
There was a problem hiding this comment.
Since we're inside ImageViewer, can we bake the check for this.options.enableAnnotationsImageDiscoverability inside this method to avoid having to pass in a string?
| }; | ||
| image.options.enableAnnotationsImageDiscoverability = enableAnnotationsImageDiscoverability; | ||
|
|
||
| expect(image.isDiscoverabilityEnabled('enableAnnotationsImageDiscoverability')).toBe( |
There was a problem hiding this comment.
Can we remove the string parameter here?
| * @return {boolean} value of whether discoverability is enabled for given type | ||
| */ | ||
| isDiscoverabilityEnabled() { | ||
| const { experiences = {} } = this.options; |
There was a problem hiding this comment.
nit: We could destructure enableAnnotationsImageDiscoverability here as well
| */ | ||
| isDiscoverabilityEnabled() { | ||
| const { experiences = {} } = this.options; | ||
| const canShow = Object.values(experiences).some(experience => experience.canShow); |
There was a problem hiding this comment.
iirc, annotations is the only experience we're passing through right now, but should this be so generic? What happens if there is a new preview thumbnails siderail experience (which doesn't apply to images) would that prevent image discoverability?
There was a problem hiding this comment.
@ConradJChan - That is a good point. I think it's most likely if any experience can show we would want to disable discoverability. I also think it would be more likely to forget to add than remember to add it, if we created some sort of eligible experiences list. I think it would make sense to address this, when the time comes, but open to make the suggested change. Let me know what you think!
There was a problem hiding this comment.
I'll go ahead and merge this PR and we can make updates in a separate PR if needed!
| }; | ||
| image.options.enableAnnotationsImageDiscoverability = enableAnnotationsImageDiscoverability; | ||
|
|
||
| expect(image.isDiscoverabilityEnabled()).toBe(!canShow && !!enableAnnotationsImageDiscoverability); |
There was a problem hiding this comment.
Instead of computing the expected result and embedding logic in the test, could it be passed in as a parameter?
ghost
added
the
For Images in Preview, we noticed that the Region is default Selected.
We also want the tooltip to close when you click on the mode button