Closed shawnborton closed 1 month ago
melvin-bot[bot]
commented
5 months ago Triggered auto assignment to @muttmuure (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.
melvin-bot[bot]
commented
5 months ago :warning: It looks like this issue is labelled as a New Feature but not tied to any GitHub Project. Keep in mind that all new features should be tied to GitHub Projects in order to properly track external CAP software time :warning:
allgandalf
commented
5 months ago Can this be opened to external contributors as this is a relatively simple feature to implement :)
ghost
commented
5 months ago Move "Troubleshoot" out of About and into Settings as its own nav item
This is the a New Feature
The Troubleshoot is inside About Page i.e. here :
https://github.com/Expensify/App/blob/71e3aa4eb3be90eeadd27260dfb87a3ff67185be/src/pages/settings/AboutPage/AboutPage.tsx#L90-L94
We need to move it into InitialPage, just above Signout button here :
https://github.com/Expensify/App/blob/71e3aa4eb3be90eeadd27260dfb87a3ff67185be/src/pages/settings/InitialSettingsPage.tsx#L250-L256
Also, we need to modify the Troubleshoot completely to look like About Us Page
https://github.com/Expensify/App/blob/860cca67cd3ee3309af766dee54b97a14df19556/src/pages/settings/AboutPage/TroubleshootPage.tsx
N/A
Need Illustration to be added.
muttmuure
commented
5 months ago I think we should actually make this an External issue, since Tomasz is working on a higher value performance project
melvin-bot[bot]
commented
5 months ago Job added to Upwork: https://www.upwork.com/jobs/~01928d4373947e3fe3
melvin-bot[bot]
commented
5 months ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)
ghost
commented
5 months ago Hey @muttmuure, I have already posted a proposal and I am already working. Should I complete it and post a test branch in the proposal ?
shubham1206agra
commented
5 months ago Move "Troubleshoot" out of About and into Settings as its own nav item
New Feature request.
We need to move the Troubleshoot from inside the About Page
To InitialPage, just above the Signout button
We also need to update the CENTRAL_PANE_TO_RHP_MAPPING to
[SCREENS.SETTINGS.ABOUT]: [SCREENS.SETTINGS.APP_DOWNLOAD_LINKS],
[SCREENS.SETTINGS.TROUBLESHOOT]: [SCREENS.SETTINGS.CONSOLE],We must also update the CentralPaneNavigatorParamList to add SCREENS.SETTINGS.TROUBLESHOOT.
We also need to update TAB_TO_CENTRAL_PANE_MAPPING to add SCREENS.SETTINGS.TROUBLESHOOT.
The Troubleshoot Page design would be similar to AboutPage.
And minor changes to linkingConfig.
Test branch : https://github.com/shubham1206agra/App/tree/test-troubleshoot-pag
Screenshot
N/A
ShridharGoel
commented
5 months ago New feature.
Troubleshoot is currently set as RHP.
In linkingConfig/config.ts
Add this under NAVIGATORS.CENTRAL_PANE_NAVIGATOR
[SCREENS.SETTINGS.TROUBLESHOOT]: {
path: ROUTES.SETTINGS_TROUBLESHOOT,
exact: true,
},And remove it from NAVIGATORS.RIGHT_MODAL_NAVIGATOR.
Add this in BaseCentralPaneNavigator.tsx inside settingsScreen
[SCREENS.SETTINGS.TROUBLESHOOT]: () => require('../../../../../pages/settings/AboutPage/TroubleshootPage').default as React.ComponentType,Remove SCREENS.SETTINGS.TROUBLESHOOT from [SCREENS.SETTINGS.ABOUT] in CENTRAL_PANE_TO_RHP_MAPPING.
Add SCREENS.SETTINGS.TROUBLESHOOT in TAB_TO_CENTRAL_PANE_MAPPING
Add [SCREENS.SETTINGS.TROUBLESHOOT]: undefined; in CentralPaneNavigatorParamList.
Add below in InitialSettingsPage items list
{
translationKey: 'initialSettingsPage.aboutPage.troubleshoot',
icon: Expensicons.Lightbulb,
routeName: ROUTES.SETTINGS_TROUBLESHOOT,
},And remove it the troubleshoot option from About page.
Example code for the UI (can be polished)
<ScreenWrapper
shouldEnablePickerAvoiding={false}
shouldShowOfflineIndicatorInWideScreen
testID={TroubleshootPage.displayName}
>
<HeaderWithBackButton
title={translate('initialSettingsPage.aboutPage.troubleshoot')}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={() => Navigation.goBack()}
icon={Expensicons.Lightbulb2}
/>
<ScrollView contentContainerStyle={styles.pt3}>
<View style={[styles.flex1, isSmallScreenWidth ? styles.workspaceSectionMobile : styles.workspaceSection]}>
<Section
title={translate('initialSettingsPage.aboutPage.troubleshoot')}
isCentralPane
subtitleMuted
illustration={LottieAnimations.Desk}
titleStyles={styles.accountSettingsSectionTitle}
illustrationBackgroundColor={theme.PAGE_THEMES[SCREENS.SETTINGS.TROUBLESHOOT].backgroundColor}
>
<View style={[styles.settingsPageBody, styles.mt5]}>
<Text style={styles.mb4}>
<Text>{translate('initialSettingsPage.troubleshoot.description')}</Text>{' '}
<TextLink
style={styles.link}
onPress={() => Report.navigateToConciergeChat()}
>
{translate('initialSettingsPage.troubleshoot.submitBug')}
</TextLink>
</Text>
</View>
<View style={[styles.mr8]}>
<TestToolRow title="Client side logging">
<Switch
accessibilityLabel="Client side logging"
isOn={!!shouldStoreLogs}
onToggle={() => (shouldStoreLogs ? Console.disableLoggingAndFlushLogs() : Console.setShouldStoreLogs(true))}
/>
</TestToolRow>
</View>
<MenuItemList
menuItems={menuItems}
shouldUseSingleExecution
/>
{/* Enable additional test features in non-production environments */}
{!isProduction && (
<View style={[styles.mr8, styles.mt6]}>
<TestToolMenu />
</View>
)}
<ConfirmModal
title={translate('common.areYouSure')}
isVisible={isConfirmationModalVisible}
onConfirm={() => {
setIsConfirmationModalVisible(false);
Onyx.clear(keysToPreserve).then(() => {
App.openApp();
});
}}
onCancel={() => setIsConfirmationModalVisible(false)}
prompt={translate('initialSettingsPage.troubleshoot.confirmResetDescription')}
confirmText={translate('initialSettingsPage.troubleshoot.resetAndRefresh')}
cancelText={translate('common.cancel')}
/>
</Section>
</View>
</ScrollView>
</ScreenWrapper>
);https://github.com/ShridharGoel/ExpensifyApp/tree/test-branch-for-38594
Krishna2323
commented
5 months ago Move "Troubleshoot" out of About and into Settings as its own nav item
New feature
Steps we need to follow:
AboutPage to InitialSettingsPage with routeName.
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/pages/settings/AboutPage/AboutPage.tsx#L91
To:
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/pages/settings/InitialSettingsPage.tsx#L249SCREENS.SETTINGS.TROUBLESHOOT from places below:
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/linkingConfig/CENTRAL_PANE_TO_RHP_MAPPING.ts#L38
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx#L229
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/linkingConfig/config.ts#L179-L181SCREENS.SETTINGS.TROUBLESHOOT mapping to the places below:
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator/BaseCentralPaneNavigator.tsx#L22
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/types.ts#L104
https://github.com/Expensify/App/blob/82065804528dc66daaa6ba652ab2dea84684c619/src/libs/Navigation/types.ts#L104ShridharGoel
commented
5 months ago Updated with new UI.
ShridharGoel
commented
5 months ago Updated with link of test branch
tienifr
commented
5 months ago Move TroubleshootPage into the Settings navigation as it's own nav item, and the Troubleshoot UI should look like the About page.
This is new feature request.
Small note for reviewer: I was not the first ones to post the proposal, partly because I tried to actually get it working and implementing it locally first, so that I can write a high quality proposals and able to highlight the differences and caveats that we need to take note of (mostly in the 1 part above) instead of rushing to a proposal when I didn't have it actually working and looking like design from my local. If you look into the history you can see I'm the first to have a screenshot of the new page actually working and looking exactly like design, I had to made sure of that before I write the proposal.
Some important notes:
renderSubtitle prop to Section, because for trouble shoot page, the subtitle will contain a hyperlinkTestToolRow, so it's not similar to the AboutPage as above proposal mentionedHeader, currently we don't have that in the app so I temporarily substitute by the black light bulb iconMove the troubleshoot item here to above the sign out button here
Make sure the navigation works with the TroubleshootPage as an independent page, this part is the same as for the AboutPage
It looks completely like design now (except for the light bulb part which I explained above):
Here's the full code for the UI part
return (
<ScreenWrapper
shouldEnablePickerAvoiding={false}
shouldShowOfflineIndicatorInWideScreen
testID={TroubleshootPage.displayName}
>
<HeaderWithBackButton
title={translate('initialSettingsPage.aboutPage.troubleshoot')}
shouldShowBackButton={isSmallScreenWidth}
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}
icon={Expensicons.Lightbulb}
/>
<ScrollView contentContainerStyle={styles.pt3}>
<View style={[styles.flex1, isSmallScreenWidth ? styles.workspaceSectionMobile : styles.workspaceSection]}>
<Section
title={translate('initialSettingsPage.aboutPage.troubleshoot')}
subtitle={translate('initialSettingsPage.troubleshoot.description')}
isCentralPane
subtitleMuted
illustration={LottieAnimations.Desk}
titleStyles={styles.accountSettingsSectionTitle}
illustrationBackgroundColor={theme.PAGE_THEMES[SCREENS.SETTINGS.TROUBLESHOOT].backgroundColor}
renderSubtitle={() => <Text style={[styles.mt4]}>
<Text>{translate('initialSettingsPage.troubleshoot.description')}</Text>{' '}
<TextLink
style={styles.link}
onPress={() => Report.navigateToConciergeChat()}
>
{translate('initialSettingsPage.troubleshoot.submitBug')}
</TextLink>
</Text>}
>
<View style={[styles.flex1, styles.mt5]}>
<View style={[styles.mr8]}>
<TestToolRow title="Client side logging">
<Switch
accessibilityLabel="Client side logging"
isOn={!!shouldStoreLogs}
onToggle={() => (shouldStoreLogs ? Console.disableLoggingAndFlushLogs() : Console.setShouldStoreLogs(true))}
/>
</TestToolRow>
</View>
<MenuItemList
menuItems={menuItems}
shouldUseSingleExecution
/>
{!isProduction && (
<View style={[styles.mr8, styles.mt6]}>
<TestToolMenu />
</View>
)}
<ConfirmModal
title={translate('common.areYouSure')}
isVisible={isConfirmationModalVisible}
onConfirm={() => {
setIsConfirmationModalVisible(false);
Onyx.clear(keysToPreserve).then(() => {
App.openApp();
});
}}
onCancel={() => setIsConfirmationModalVisible(false)}
prompt={translate('initialSettingsPage.troubleshoot.confirmResetDescription')}
confirmText={translate('initialSettingsPage.troubleshoot.resetAndRefresh')}
cancelText={translate('common.cancel')}
/>
</View>
</Section>
</View>
</ScrollView>
</ScreenWrapper>NA
ShridharGoel
commented
5 months ago Updated to polish the UI even more.
ShridharGoel
commented
5 months ago Note that I've also added the needed light bulb in Expensicons.tsx, which can be seen in my test branch.
tienifr
commented
5 months ago Since this is feature implementation, I'd be happy to provide the full test branch for my proposal upon request (although you can see the full result attached in the proposal from the start) 👍
shubham1206agra
commented
5 months ago Updated due to people posting complete diff for no reason (which is discouraged), and this is not a good practice.
ghost
commented
5 months ago Updated Proposal with test branch since in the initial comment it was asked to be assigned to
ghost
commented
5 months ago Proposal
Updated due to people posting complete diff for no reason (which is discouraged), and this is not a good practice.
In the original proposal template it is mentioned not to propose diffs but still folks are doing it.
alitoshmatov
commented
5 months ago Thank you everyone for your proposal, I see all the proposals are similar in some way. There are a lot of updating in all proposals which is confusing, I try to be as fair as possible
alitoshmatov
commented
5 months ago @godofoutcasts94 I see you have a problem in section subtitle where there should be link to submit a bug
ghost
commented
5 months ago @godofoutcasts94 I see you have a problem in section subtitle where there should be link to submit a bug
I will have a look and rectify it maybe I have missed in the proposal. Will fix it in the proposal and get back to you
alitoshmatov
commented
5 months ago @shubham1206agra Your proposal looks good, same approach as other proposals except you have opted out section subtitle, and just put text inside section which is why its style is a little bit different.
alitoshmatov
commented
5 months ago @ShridharGoel Your proposal also looks good, and almost the same as @shubham1206agra 's proposal
alitoshmatov
commented
5 months ago @Krishna2323 Your proposal is also similar to others except it lacks some details like how you handle section subtitle with a link
alitoshmatov
commented
5 months ago @tienifr Your proposal is also almost the same as others. One difference you approached section subtitle a little bit differently by adding, renderSubtitle.
alitoshmatov
commented
5 months ago Thank you everyone. To simplify and move forward, I narrowed down two potential proposals. @shubham1206agra and @tienifr 's proposals look good and provide potential solution to section subtitle which should have a link in it. I see both of your point and note sure which is better. Would love to hear from you some arguments and details related to this point of your solution, any advantages, any similar use cases would help. Look forward for your analysis
ghost
commented
5 months ago so how about my proposal? @alitoshmatov, I can fix and come back to you, still my proposal won't be considered ?
alitoshmatov
commented
5 months ago @godofoutcasts94 If you have different approach to the problem we are facing with section subtitle, I would be happy to re-consider your proposal.
shubham1206agra
commented
5 months ago Thank you everyone. To simplify and move forward, I narrowed down two potential proposals. @shubham1206agra and @tienifr 's proposals look good and provide potential solution to section subtitle which should have a link in it. I see both of your point and note sure which is better. Would love to hear from you some arguments and details related to this point of your solution, any advantages, any similar use cases would help. Look forward for your analysis
@alitoshmatov I did not opt for changes in the section subtitle as this would add complicated logic for the place it is used once. And I can adjust the style later in this so that it will look identical to the ideal solution. And I do not like adding logic for subtitle rendering as we have to write the JSX code for subtitle anyway. And we can adjust this in children prop easily.
tienifr
commented
5 months ago @shubham1206agra and @tienifr 's proposals look good and provide potential solution to section subtitle which should have a link in it.
@alitoshmatov Section subtitle is essentially a "customized subtitle", which has slightly more complex UI than a regular subtitle. Here're the reasons I believe it's better to have a renderSubtitle:
Section and it's not a new, distinct component in the main page (as is the case in @shubham1206agra). Section component, it will take effect immediately in the renderSubtitle. If instead the subtitle is a separate component in 1 single page, it will easily be missed out in refactoring, causing UI inconsistency, which we'll then have to create a new GH issue to fixSection to allow using the renderSubtitle instead of the subtitle, if renderSubtitle prop exists), and it brings all the benefit aboveSection UI component, and a developer asked me how they can customize the subtitle that allows link click, I'd surely provide him a renderSubtitle prop instead of telling him "just omit the prop and try to replicate the subtitle UI in the page you want to use it". Would you? 😄 shubham1206agra
commented
5 months ago It can be reused easily in other pages, I'm quite sure having a page subtitle with a clickable link is a very common use case that will appear again in the future (soon). If we opt for a case-by-case fix now, we'll have to duplicate this case-by-case fix in future pages.
I don't think this is planned for other pages in the near future (I just checked Figma designs for upcoming features). So, this looks like a one-off case for now. Maybe in the very distant future, it may occur 1-2 times, but I don't think this is even worth it to introduce such logic.
This sub-component customization pattern is used everywhere else in the app, like here, here, or here
These are not good examples at all as these components do not have alternative choices to this approach.
It's better modular component design. If I was to write an open source library that exposes a Section UI component, and a developer asked me how they can customize the subtitle that allows link click, I'd surely provide him a renderSubtitle prop instead of telling him "just omit the prop and try to replicate the subtitle UI in the page you want to use it". Would you? 😄
Is this a super generalized library? I don't think so. This is so a niche case we may not want to handle it inside the Section component.
It's not complicated (only 1-2 lines of code more in the Section to allow using the renderSubtitle instead of the subtitle, if renderSubtitle prop exists), and it brings all the benefit above
This will then require changes to all uses Section component that uses subtitles. And it may introduce extra View component which is not very DRY in the first place.
@alitoshmatov Shouldn't the details matter too much in this case? We are just migrating the component here to a different place. These things can be decided later. And, how does rendering subtitles make the new proposal altogether in this issue? The styles can be adjusted later easily, which should not matter in the proposal too much.
tienifr
commented
5 months ago This will then require changes to all uses Section component that uses subtitles. And it may introduce extra View component which is not very DRY in the first place.
@shubham1206agra This won't require any changes to other Section component that uses subtitles, the other places will still pass the existing subtitle prop, it doesn't pass the renderSubtitle so the custom subtitle will not be rendered.
We've given our opinions, let's see what @alitoshmatov thinks!
shubham1206agra
commented
5 months ago I still think this is kind of a one off case, and we should not be updating the component too much.
@alitoshmatov I still believe we should not nitpick details too much. I can too nitpick about the alignment of menuitem is off in other proposals.
ghost
commented
5 months ago Updated test branch
ShridharGoel
commented
5 months ago Your proposal also looks good, and almost the same
Can you also consider the level of details and edit timestamps? My proposal seems to be the 1st to have complete navigation + UI.
melvin-bot[bot]
commented
5 months ago 📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸
alitoshmatov
commented
5 months ago Thank you everyone for your proposal I understand that section subtitle might seem small detail but I think it is important enough that we should approach it with ideal solution, also all proposal being almost the same and a lot of edits made the review process difficult. I tried to be as reasonable as possible when choosing proposal.
alitoshmatov
commented
5 months ago I do think that renderSubtitle suggested by @tienifr is the correct approach and I agree with their points. It might seem one-off thing but as soon as we need another similar case where section subtitle require more than just a plain text we run into problem, that is why for the sake of future proofing we should apply this solution.
We can go with @tienifr 's proposal
C+ reviewed 🎀 👀 🎀
melvin-bot[bot]
commented
5 months ago Triggered auto assignment to @danieldoglas, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
danieldoglas
commented
5 months ago big discussions on this one. Thanks for curating it all @alitoshmatov.
@shawnborton, as @tienifr pointed, can you please send us the lightbulb illustration?
We need to supply a Lightbulb illustration as the icon in the Header, currently we don't have that in the app so I temporarily substitute by the black light bulb icon
assigning @tienifr
melvin-bot[bot]
commented
5 months ago 📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
melvin-bot[bot]
commented
5 months ago 📣 @tienifr 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑💻 Keep in mind: Code of Conduct | Contributing 📖
shawnborton
commented
5 months ago Here's the illustration you need: simple-illustration__lightbulb.svg.zip
shawnborton
commented
4 months ago @tienifr looks like we have a small regression on iOS - notice how the illustration is sitting too far down compared to desktop?
iOS:
Desktop:
Can you please submit a follow up PR to fix?
shawnborton
commented
4 months ago Friendly bump @tienifr
tienifr
commented
4 months ago @shawnborton Sorry for my lateness. I am checking it now
We want to take the Troubleshoot page:
And move it into the Settings navigation as it's own nav item, right above the Sign Out option at the bottom. When doing this, the Troubleshoot UI should look like the About page, where we use the card style and a nice header with an illustration + h2:
cc @TMisiukiewicz since I think you originally implemented this? This would just be a follow up to that.
cc @muttmuure @Expensify/design
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @muttmuure