Prepare Side Panel for Stage 4 #59740
Conversation
|
Anything needed from the design team in terms of a review, or is this mostly just restructuring some code? |
@shawnborton Just code restructuring this time |
|
Assigning @ikevin127 as a one-off for the review and checklist. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.mp4Android: mWeb Chromeandroid-mweb.mp4iOS: Nativeios.mp4iOS: mWeb Safariios-mweb.mp4MacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
Please ping me on Slack if you have any questions @ikevin127 as I'm wrapping up for the day. |
| const [sidePanelNVP] = useOnyx(ONYXKEYS.NVP_SIDE_PANEL); | ||
| const [language] = useOnyx(ONYXKEYS.NVP_PREFERRED_LOCALE); | ||
| const [canUseHelpSidePanel = false] = useOnyx(ONYXKEYS.BETAS, {selector: (betas) => !!betas?.includes(CONST.BETAS.HELP_SIDE_PANEL)}); | ||
| const [canUseHelpSidePanel = false] = useOnyx(ONYXKEYS.BETAS, {selector: Permissions.canUseHelpSidePanel}); |
There was a problem hiding this comment.
To confirm, we're doing this because using usePermissions() still made some tests fail?
There was a problem hiding this comment.
Yes, exactly. And I forgot to use canUseHelpSidePanel in the previous PR
| isExact?: boolean; | ||
| }; | ||
|
|
||
| const helpContentMap: HelpContent = { |
There was a problem hiding this comment.
@francoisl I'm assuming we're not adding translations for all this english copy ?
There was a problem hiding this comment.
@francoisl Another thing here related to PRs Explanation of Change mentioning:
Create a single App/src/components/SidePanel/HelpContent/helpContentMap.tsx file
- Just specify everything directly in that one file -- no imports. It's all going to be loaded at runtime anyway, and this file will be auto-generated eventually
we still have imports here like BulletList, ExpandableHelp, Text, and TextLink which are imported rather than being defined directly in the file.
Would these pose a problem given that this file will be loaded at runtime / auto-generated ?
There was a problem hiding this comment.
Good point. I'm trying to think how we could avoid any of these imports. We could probably have a dumbed-down version of the map, where each element is defined by a type key so that we can differentiate between what's a title, what's a subsection, what's a link, what's a bullet list, etc. And then some other helper ingests that map and converts each element to a ReactNode on the fly.
Rough example:
const helpContentMap = {
example: [
{
type: 'title',
text: 'Chat',
},
{
type: 'text',
text: 'Chat is the foundation of New Expensify. Every [....]',
},
{
type: 'bulletlist',
title: 'The core of the chat are its comments, which come in many forms:',
items: [
{
type: 'text',
text: 'Rich text messages stored securely and delivered via web, app, email',
}
]
},
]
}But even with this, if you want to add some style variations, or nested elements with different styles like we have here:
<Text style={styles.textNormal}>
<Text style={styles.textBold}>Text</Text> - Rich text messages stored securely and delivered via web, app, email, or SMS.
</Text>,the format becomes more complex.
Thoughts? This is also probably something we can iterate on as we go.
| </Text>, | ||
| <Text style={styles.textNormal}> | ||
| <Text style={styles.textBold}>Mention</Text> - Invite or tag anyone in the world to any chat by putting an @ in front of their email address or phone | ||
| number (e.g., <Text style={styles.textBold}>@awong@marslink.web</Text>, or <Text style={styles.textBold}>@415-867-5309</Text>). |
There was a problem hiding this comment.
These are dummy email and phone numbers, right? I'm seeing a ton of various matches for the phone number online so I assume, but just double-checking.
There was a problem hiding this comment.
I think so, we might want to change it 😅
| <Text style={styles.textBold}>Collect</Text> workspaces start at $5/member, and include all the basics for running a small business. | ||
| </Text>, | ||
| <Text style={styles.textNormal}> | ||
| <Text style={styles.textBold}>Control</Text> workspaces start at $9/member, and provide advanced capabilities, more powerful accounting sync, and |
There was a problem hiding this comment.
Out of scope for this PR, but maybe we should consider using the pricing constants instead (e.g. SUBSCRIPTION_PRICES) down the line
There was a problem hiding this comment.
Let's not use any CONST due to this:
Don't use constants (eg, `:${CONST.REPORT.HELP_TYPE.POLICY_ADMINS}) because the code we generate isn't going to use them either.
| }, | ||
| }, | ||
| home: { | ||
| content: ({styles}: {styles: ThemeStyles}) => ( |
There was a problem hiding this comment.
This whole function returns the same thing as at the root (lines 30-148). Thoughts on reusing a const that's defined beforehand instead? We don't necessarily have to do it now, but as it stands I'm thinking that down the line, we might want to edit the copy and risk forgetting to edit both places at the same time.
There was a problem hiding this comment.
I wanted to keep it simple due to the fact this file is going to be generated going forward, but I totally agree with you.
We have an answer here! @francoisl @ikevin127 let's proceed with a merge 😄 |
|
🚀 Deployed to staging by https://github.com/francoisl in version: 9.1.24-2 🚀
|
|
@francoisl @shawnborton Can you pls help us with QA steps here? |
@mvtglobally I assume Expensifail accounts have side panel beta enabled, right? Please verify that Side Panel opens on various pages when you have beta, the appropriate content is displayed according to this document. For a non-beta account just verify that help button is not displayed anywhere. |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.24-10 🚀
|
5 participants
Explanation of Change
Fixed Issues
$ #58732
PROPOSAL: https://expensify.slack.com/archives/C07NZ8B1VTQ/p1743366805812629
Tests
Offline tests
N/A
QA Steps
Same as tests
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.webm
Android: mWeb Chrome
and-web.webm
iOS: Native
ios.mp4
MacOS: Chrome / Safari
web.mov