[Internal QA]: feat: Unmask tooltip copy for Fullstory #55650
Conversation
twilight2294
changed the title
|
@twilight2294 Any update on this? |
|
Should we mark this no QA ? I don't think we can test this one, if so I can open this for review |
|
@twilight2294 I think you're right, based on this PR they had to do |
twilight2294
changed the title
| @@ -122,12 +122,20 @@ function BaseGenericTooltip({ | |||
|
|
|||
There was a problem hiding this comment.
@jjcoffee do you have any idea if we have FS sessions for Native devices as well, if so then i need to update index.native.tsx as well, let me know
There was a problem hiding this comment.
I'm not sure about this, maybe @puneetlath knows?
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
thanks for the review @puneetlath , I updated the code accordingly |
|
@puneetlath I'm not too familiar with how we use FS, but based on this, it might need to be on staging before it can be tested? |
|
I was testing locally, but it's possible I was doing something wrong. What about the constant. Shall we change that? |
|
Yes, I like that idea! Let's use the |
| <View style={[styles.mh100, onboardingIsMediumOrLargerScreenWidth && StyleUtils.getWidthStyle(width), safeAreaPaddingBottomStyle]}> | ||
| <View | ||
| style={[styles.mh100, onboardingIsMediumOrLargerScreenWidth && StyleUtils.getWidthStyle(width), safeAreaPaddingBottomStyle]} | ||
| fsClass="fs-unmask" |
There was a problem hiding this comment.
@jjcoffee @puneetlath if we use const then the above prop will be:
fsClass=`${CONST.FULL_STORY.UNMASK}`This is what is expected right ? we would need to include `` because we are using a const, asking just to be sure
There was a problem hiding this comment.
I think you just want to do fsClass={CONST.FULL_STORY.UNMASK}? Please also see this comment, I think we need to use parseFSAttributes in order for this to work.
There was a problem hiding this comment.
you sure that it would assign fsClass an string value? because i think fsClass will only need string values
There was a problem hiding this comment.
Please also see #55766 (comment), I think we need to use parseFSAttributes in order for this to work.
I'll take a look
There was a problem hiding this comment.
you sure that it would assign fsClass an string value? because i think fsClass will only need string values
@twilight2294 I'm not sure what you mean here, can't you just test it and see if the class gets added? I don't see why it wouldn't, however!
Also bump on the parseFSAttributes question.
There was a problem hiding this comment.
I updated, working on the failing unit tests, do you have any idea what might have caused it
There was a problem hiding this comment.
I'm not sure but if I comment out useLayoutEffect(parseFSAttributes, []) only on ProductTrainingContext, the test no longer fails.
There was a problem hiding this comment.
umm, testing what can be done here
There was a problem hiding this comment.
@jjcoffee I got the fix, we didn't mock FullStory lib in the unit tests like we do here:
App/tests/perf-test/ReportActionsList.perf-test.tsx
Lines 29 to 33 in 631006e
So the tests were failing, I have updated it now 🥳
Apologies for the delay here, I will push the update now
| <View style={[styles.mh100, onboardingIsMediumOrLargerScreenWidth && StyleUtils.getWidthStyle(width), safeAreaPaddingBottomStyle]}> | ||
| <View | ||
| style={[styles.mh100, onboardingIsMediumOrLargerScreenWidth && StyleUtils.getWidthStyle(width), safeAreaPaddingBottomStyle]} | ||
| fsClass={CONST.FULL_STORY.UNMASK} |
There was a problem hiding this comment.
yes thanks, will update
|
Any update here? |
Working on fixing the failing test |
|
@jjcoffee @puneetlath all yours, apologies for the delay |
| @@ -264,6 +265,9 @@ function FeatureTrainingModal({ | |||
| onConfirm?.(); | |||
| }, [onConfirm, closeModal]); | |||
|
|
|||
| // Parse Fullstory attributes on initial render | |||
There was a problem hiding this comment.
Can we please update this comment to say "why", not just "what"?
| @@ -36,6 +37,9 @@ function OnboardingWelcomeVideo() { | |||
| const StyleUtils = useStyleUtils(); | |||
| const {shouldUseNarrowLayout} = useResponsiveLayout(); | |||
|
|
|||
| // Parse Fullstory attributes on initial render | |||
There was a problem hiding this comment.
Can we please update this comment to say "why", not just "what"?
| @@ -173,11 +174,15 @@ const useProductTrainingContext = (tooltipName: ProductTrainingTooltipName, shou | |||
| }; | |||
| }, [tooltipName, registerTooltip, unregisterTooltip, shouldShow]); | |||
|
|
|||
| // Parse Fullstory attributes on initial render | |||
| @@ -101,15 +102,26 @@ function BaseGenericTooltip({ | |||
| }); | |||
| }); | |||
|
|
|||
| // Parse Fullstory attributes on initial render | |||
| @@ -120,12 +121,16 @@ function BaseGenericTooltip({ | |||
| return StyleUtils.getTooltipAnimatedStyles({tooltipContentWidth: contentMeasuredWidth, tooltipWrapperHeight: wrapperMeasuredHeight, currentSize: animation}); | |||
| }); | |||
|
|
|||
| // Parse Fullstory attributes on initial render | |||
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.98-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.98-8 🚀
|
Explanation of Change
Fixed Issues
$ #55425
PROPOSAL: #55425 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop