-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
clean up existing Product training tooltips #53396
clean up existing Product training tooltips #53396
Conversation
…tegrate product training context in ReportActionCompose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to review this since I reviewed #53225 and have context
Please start when you can, i'll completed checklist in couple hours/tomorrow |
…isibility conditions
…te workspace chat translation
@rayane-djouah I have resolved/responded to your review comments above, will you be able to re-review. |
…in English and Spanish
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good! Testing..
@ishpaul777, BUG: The tooltips do not display the correct colors in dark mode: |
@ishpaul777, BUG: The |
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-10.at.9.09.20.PM.movAndroid: mWeb ChromeScreen.Recording.2024-12-10.at.9.14.22.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-12-10.at.21.01.16.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-12-10.at.21.03.02.mp4MacOS: Chrome / SafariScreen.Recording.2024-12-10.at.8.15.36.PM.movScreen.Recording.2024-12-10.at.8.26.47.PM.movMacOS: DesktopScreen.Recording.2024-12-10.at.8.17.37.PM.mov |
… and add EducationalTooltipContent component
…ltipContent component
reproducable on main as well with QAB tooltip also Record_2024-12-12-00-01-48.mp4 |
Screenshots/VideosAndroid: NativeScreen.Recording.2024-12-11.at.10.35.23.PM.moviOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-12-11.at.22.59.24.mp4MacOS: Chrome / SafariScreen.Recording.2024-12-11.at.11.01.49.PM.mov |
i have updated translation and hide this behind beta for now! |
Reported two bugs that are reproducible on main also: https://expensify.slack.com/archives/C049HHMV9SM/p1733957083495449 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tests well 👍
/** | ||
* When user dismisses the conciergeLHNGBR product training tooltip, we store the timestamp here. | ||
*/ | ||
conciergeLHNGBR: Date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use the constants from Const file for these? Or do we not typically do that for types files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like this @puneetlath ?
import CONST from '@src/CONST';
const {CONCEIRGE_LHN_GBR, RENAME_SAVED_SEARCH, WORKSAPCE_CHAT_CREATE, QUICK_ACTION_BUTTON} = CONST.PRODUCT_TRAINING_TOOLTIP_NAMES;
type DismissedProductTraining = {
/**
* When user dismisses the nudgeMigration Welcome Modal, we store the timestamp here.
*/
[CONST.MIGRATED_USER_WELCOME_MODAL]: Date;
/**
* When user dismisses the conciergeLHNGBR product training tooltip, we store the timestamp here.
*/
[CONCEIRGE_LHN_GBR]: Date;
.......
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes exactly. What do you think?
@@ -0,0 +1,67 @@ | |||
import type {ValueOf} from 'type-fest'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all caps filename seems unusual. Is that usually how we name these?
✋ 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.76-0 🚀
|
This PR is failing because of issue #54127 |
Explanation of Change
Fixed Issues
$ #53086
PROPOSAL:
Tests
2.( On desktop, resize screen to mobile size) verify Tooltip on Concierge is displayed
3.Create a workpace, go back and open FAB menu, Verify that a tooltip on QAB is visibile
Offline tests
QA Steps
Same as test steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-12-09.at.10.49.16.PM.mov
Android: mWeb Chrome
iOS: Native
Screen.Recording.2024-12-09.at.10.09.07.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-12-09.at.10.16.32.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-12-09.at.10.13.41.PM.mov
MacOS: Desktop
Screen.Recording.2024-12-09.at.10.58.01.PM.mov