feat(insights): Added support for Advanced Content Insights - #3116
Conversation
|
|
8dd3258 to
405b430
Compare
43a2134 to
6a81727
Compare
6a81727 to
35df164
Compare
tjuanitas
left a comment
There was a problem hiding this comment.
could you add screenshots of these changes?
I think it would be helpful in the future to split changes like these into smaller PRs
35df164 to
753c990
Compare
4418d9e to
9e255ed
Compare
9e255ed to
4514b3c
Compare
4514b3c to
d8315b5
Compare
tjuanitas
left a comment
There was a problem hiding this comment.
took another pass through since I saw the PR was still open
|
|
||
| interface Props { | ||
| hasTooltip?: boolean; | ||
| isAdvancedContentInsightsChecked?: boolean; |
There was a problem hiding this comment.
this prop could be isChecked since AdvancedContentInsights is part of the component name so it's not needed to include it in the prop name
note: the change would only be for this component. it still makes sense to keep it in UnifiedShareForm
There was a problem hiding this comment.
thanks! one more place sorry:
isChecked={isAdvancedContentInsightsChecked}
bf909de to
6cd0267
Compare
| ); | ||
| }); | ||
|
|
||
| describe('getThumbnail()', () => { |
There was a problem hiding this comment.
Should we add a test to ensure that the thumbnail has the properties that we are expecting. I notice we check for null but it might be helpful to test that the thumbnail is going to be returned properly.
There was a problem hiding this comment.
Hi, for the thumbnail We are testing that the viewer.getThumbnail is called. So for unit testing I think is the way to go. That viewer function will need to unit test if the thumbnail is being returned properly. Also, this function is going to be removed before going GA since we are going to switch the thumbnail provider (not gonna use PDFjs)
| allShareRestrictionWarning; | ||
|
|
||
| const showContentInsightsToggle = | ||
| onAdvancedContentInsightsToggle && |
There was a problem hiding this comment.
Parts of this conditional are the exact same as the ones in the statement above. I'm guessing that combination of conditions has greater meaning in the context of the form than they do individually with regard to whether or not we want to show those elements. Do you think it would be a good idea to extract those common conditions out into their own function? It might make the code more readable and easy to understand.
There was a problem hiding this comment.
Hey @bfoxx1906 , @tjuanitas . I added some logic regarding this comment @bfoxx1906 added. I agree with him that it would be better to make that condition more readable. PTAL and please fell free to share your thoughts
There was a problem hiding this comment.
Hi @tjuanitas and @bfoxx1906 . Bumping this to get final feedback for this change. Thanks
91f91f5 to
c010e1e
Compare
| !isInviteSectionExpanded && | ||
| !showCollaboratorList && | ||
| allShareRestrictionWarning; | ||
| const isMainUSMSectionShown = !isEmailLinkSectionExpanded && !isInviteSectionExpanded && !showCollaboratorList; |
There was a problem hiding this comment.
not sure if these are better names but what about: isInitialStateShown or hasExpandedSections (requires inverting the boolean logic)
"main USM section" sounds a little ambiguous. i.e. what is the main section?
There was a problem hiding this comment.
Thanks @tjuanitas . Actually I like the hasExpandedSections better so I updated the code
- Change ContentPreview test to make it more simple - Removed the flowTypes from advanced content insights - Fix an issue with reference object comparison instead of deep comparison
- Change Variable name from isMainUSMSectionShown to hasExpandedSections and inverted boolean logic - Modify onRequestClose on ModalDialog to follow the same approach we are using onRequestBack
702c2b1 to
a5727a7
Compare
Uh oh!
There was an error while loading. Please reload this page.