Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[No QA] [Manager McTest] Update our exisitingEducationalTooltip component to display the new buttons (Try it out/No thanks) #56239

Merged
merged 2 commits into from
Feb 11, 2025
Merged

[No QA] [Manager McTest] Update our exisitingEducationalTooltip component to display the new buttons (Try it out/No thanks) #56239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df566ce
add an option to display action buttons in tooltips
JKobrynski Feb 3, 2025
175879f
Merge remote-tracking branch 'upstream/main' into JKobrynski/feat/550…
JKobrynski Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/components/ProductTrainingContext/TOOLTIPS.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type TooltipData = {
name: ProductTrainingTooltipName;
priority: number;
shouldShow: (props: ShouldShowConditionProps) => boolean;
shouldRenderActionButtons?: boolean;
};

const TOOLTIPS: Record<ProductTrainingTooltipName, TooltipData> = {
Expand Down Expand Up @@ -129,6 +130,7 @@ const TOOLTIPS: Record<ProductTrainingTooltipName, TooltipData> = {
name: SCAN_TEST_TOOLTIP,
priority: 900,
shouldShow: () => false,
shouldRenderActionButtons: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JKobrynski I think we need to add shouldRenderActionButtons to WORKSAPCE_CHAT_CREATE tooltip instead of this place

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change that if you want, but the purpose of this PR is just to allow us to display the action buttons, the actual logic for the new tooltip flow will be introduced later! Wdyt? Should we make that change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JKobrynski But currently it doesn't match your test step

  1. Make sure you can see the tooltip next to composer (if not, make sure you check Prerequisites above)
  2. Check if you can also see the new buttons (Try it out and No thanks)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I change this, all users that sign up to a new account will see these buttons that will do nothing because the functionality is not implemented yet. In the ** Prerequisites** section I instructed reviewers to enable this for WORKSAPCE_CHAT_CREATE so they can see it on their own machine and verify if it looks right. We don't want to permanently change it, it's just for testing purposes.

},
};

Expand Down
58 changes: 39 additions & 19 deletions src/components/ProductTrainingContext/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React, {createContext, useCallback, useContext, useEffect, useMemo, useState} from 'react';
import {View} from 'react-native';
import {useOnyx} from 'react-native-onyx';
import Button from '@components/Button';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import Text from '@components/Text';
Expand Down Expand Up @@ -176,35 +177,54 @@ const useProductTrainingContext = (tooltipName: ProductTrainingTooltipName, shou
const renderProductTrainingTooltip = useCallback(() => {
const tooltip = TOOLTIPS[tooltipName];
return (
<View style={[styles.alignItemsCenter, styles.flexRow, styles.justifyContentCenter, styles.flexWrap, styles.textAlignCenter, styles.gap3, styles.p2]}>
<Icon
src={Expensicons.Lightbulb}
fill={theme.tooltipHighlightText}
medium
/>
<Text style={[styles.productTrainingTooltipText, styles.textWrap, styles.mw100]}>
{tooltip.content.map(({text, isBold}) => {
const translatedText = translate(text);
return (
<Text
key={text}
style={[styles.productTrainingTooltipText, isBold && styles.textBold]}
>
{translatedText}
</Text>
);
})}
</Text>
<View>
<View style={[styles.alignItemsCenter, styles.flexRow, styles.justifyContentCenter, styles.flexWrap, styles.textAlignCenter, styles.gap3, styles.p2]}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get a design review before we merge this- I'm thinking we can just use screenshots since we can't adhoc build this rn?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it back, let's wait on the adhoc when we go live instead so we don't need duplicate reviews

<Icon
src={Expensicons.Lightbulb}
fill={theme.tooltipHighlightText}
medium
/>
<Text style={[styles.productTrainingTooltipText, styles.textWrap, styles.mw100]}>
{tooltip.content.map(({text, isBold}) => {
const translatedText = translate(text);
return (
<Text
key={text}
style={[styles.productTrainingTooltipText, isBold && styles.textBold]}
>
{translatedText}
</Text>
);
})}
</Text>
</View>
{!!tooltip?.shouldRenderActionButtons && (
<View style={[styles.alignItemsCenter, styles.justifyContentBetween, styles.flexRow, styles.ph1, styles.pb1]}>
<Button
success
text={translate('productTrainingTooltip.scanTestTooltip.tryItOut')}
style={[styles.flex1, styles.ph1]}
/>
<Button
text={translate('productTrainingTooltip.scanTestTooltip.noThanks')}
style={[styles.flex1, styles.ph1]}
/>
</View>
)}
</View>
);
}, [
styles.alignItemsCenter,
styles.flex1,
styles.flexRow,
styles.flexWrap,
styles.gap3,
styles.justifyContentBetween,
styles.justifyContentCenter,
styles.mw100,
styles.p2,
styles.pb1,
styles.ph1,
styles.productTrainingTooltipText,
styles.textAlignCenter,
styles.textBold,
Expand Down
2 changes: 2 additions & 0 deletions src/languages/en.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5825,6 +5825,8 @@ const translations = {
part6: 'Now,',
part7: ' submit your expense',
part8: ' and watch the magic happen!',
tryItOut: 'Try it out',
noThanks: 'No thanks',
},
},
discardChangesConfirmation: {
Expand Down
2 changes: 2 additions & 0 deletions src/languages/es.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6342,6 +6342,8 @@ const translations = {
part6: 'Ahora,',
part7: ' envía tu gasto y',
part8: ' ¡observa cómo ocurre la magia!',
tryItOut: 'Prueba esto',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JKobrynski Did we confirm this translation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked for a confirmation here but got no replies, I used JaimeGPT to translate it

noThanks: 'No, gracias',
},
},
discardChangesConfirmation: {
Expand Down
Loading