feat: About Page #2926

pranshuchittora · 2021-05-14T12:54:50Z

Details

About Page

Fixed Issues

Closes #2811

TODO

QA Steps / Testing Strategy 🧪

About Page

Make sure the app version is correct (from package.json).
App Download Links opens a new screen
Direct links should open the browser window (not webview).
Report a bug should redirect to Concierge chat.

App Download Links Page

Links are correct.
Links should work

Miscellaneous

Check for icons as per the attached design

Tested On

Screenshots

iOS

Simulator.Screen.Recording.-.iPhone.11.-.2021-05-15.at.15.05.34.mp4

Android

Peek.2021-05-15.15-48.mp4

Desktop App

Screen.Recording.2021-05-15.at.3.11.38.PM.mov

Web

Screen.Recording.2021-05-15.at.3.06.51.PM.mov

github-actions · 2021-05-14T12:55:12Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

pranshuchittora · 2021-05-14T12:59:36Z

@Julesssss need some clarification / help regarding the best practices and other stuff:

Currently the MenuItem supports only shouldShowRightArrow prop which shows the arrow icon. For this feature, we need to change the icon from the arrow to link. So I am thinking of exposing a new prop called icon which accepts an icon component and overrides the arrow icon.
What’s the ideal place to house local styling.
App store link is not working, that’s weird.
Report a bug -> Redirect to the user's 1:1 chatReport with Concierge on Expensify.cash
```
I didn’t get this, can you pls elaborate on this point.
```

Once these gets resolved I will start writing the tests and then we can go ahead with review and merging.
Thanks :)

pranshuchittora · 2021-05-14T13:01:09Z

I have read the CLA Document and I hereby sign the CLA

Julesssss · 2021-05-14T13:59:55Z

Hi @pranshuchittora, I'd suggest that you join the contributor's Slack channel to ask the more general questions, as you'll likely get faster answers. Details should be in the UpWork message.

So I am thinking of exposing a new prop called icon which accepts an icon component and overrides the arrow icon.

That sounds good to me, we can probably replace the shouldShow prop and simply check if an icon is passed. I think the Picker Component might have a similar prop that you can check out.

What’s the ideal place to house local styling.

We shouldn't declare inline styles, but should define objects in styles.js.

I'd ask about 3 in the Slack channel, someone should know.

Report a bug -> Redirect to the user's 1:1 chatReport with Concierge on Expensify.cash

So in this case we want to navigate to the chat with Concierge (our support). If you log into the app you should see a conversation with Concierge -- this is the chat we want to navigate the user to.

pranshuchittora · 2021-05-14T14:05:20Z

Thanks @Julesssss

So in this case we want to navigate to the chat with Concierge (our support). If you log into the app you should see a conversation with Concierge -- this is the chat we want to navigate the user to.

To perform the following navigation is there any fixed id/route for this chat?

Julesssss · 2021-05-14T14:24:33Z

To perform the following navigation is there any fixed id/route for this chat?

The chatReportID will be different for each user, but we should always have this data stored locally in Onyx. You could retrieve all reports from Onyx and filter for the concierge@expensify.com email address to find the correct report data:

pranshuchittora · 2021-05-14T14:32:22Z

To perform the following navigation is there any fixed id/route for this chat?

The chatReportID will be different for each user, but we should always have this data stored locally in Onyx. You could retrieve all reports from Onyx and filter for the concierge@expensify.com email address to find the correct report data:

Thanks for the heads-up but I am not familiar with Onyx can you pls help me with the query

Julesssss · 2021-05-14T14:43:12Z

Thanks for the heads-up but I am not familiar with Onyx can you pls help me with the query

A lot of Components within the app have examples of retrieving data from Onyx. Here's some info to get you started.

Here's an example of a Component that retrieves all chatReports from storage. You would then need to search this data to find the report where concierge@expensify.com is the other participant
For further questions and help with Onyx it will be better to ask in the shared Slack channel

pranshuchittora · 2021-05-14T15:09:44Z

Thanks for the heads-up but I am not familiar with Onyx can you pls help me with the query

A lot of Components within the app have examples of retrieving data from Onyx. Here's some info to get you started.

Here's an example of a Component that retrieves all chatReports from storage. You would then need to search this data to find the report where concierge@expensify.com is the other participant

For further questions and help with Onyx it will be better to ask in the shared Slack channel

Thanks a lot.
I have sent the email to join the slack, can you amplify the request pls.

pranshuchittora · 2021-05-14T15:18:33Z

Is this correct?

Simulator.Screen.Recording.-.iPhone.11.-.2021-05-14.at.20.45.36.mp4

pranshuchittora · 2021-05-14T15:39:11Z

@Julesssss this PR is up for review

Simulator.Screen.Recording.-.iPhone.11.-.2021-05-14.at.21.02.23.mp4

shawnborton · 2021-05-14T15:55:33Z

src/pages/settings/AboutPage.js

+            <View style={styles.pageWrapper}>
+                <View style={[styles.settingsPageBody, styles.mb6]}>
+                    <Logo height={100} />
+                    <Text style={[styles.textLabel, styles.alignSelfCenter, {marginVertical: 10}]}>


You should be able to use a utility style for the margin rather than using an inline style here. So for example, we might already have something like mv-2 or mv-3. Check them out here: https://github.com/Expensify/Expensify.cash/blob/main/src/styles/utilities/spacing.js It looks like you might need to create the style you need for mv-X or alternatively, just use mt-2 mb-2. As a heads up, we do everything as an increment of the unit of 4.

Can you also give this version a muted color? You can simply add styles.colorMuted

Thanks for the review @shawnborton, I have added mv[1,2,3,4,5]. I think this looks more maintainable and clean.

shawnborton · 2021-05-14T15:57:20Z

src/languages/en.js

    },
    initialSettingsPage: {
        settings: 'Settings',
+        about: 'About',
+        aboutPage: {
+            appDownloadLinks: 'App Download Links',


I think this should be App download links so it follows the same capitalisation format as the others. @trjExpensify how do you feel about that?

Agreed 👍

Julesssss · 2021-05-14T15:58:25Z

That looks correct @pranshuchittora. I will get to the PR on Monday as I am on BST.

In the meantime, could you add a screenshot for each platform, thanks. -- I will add this step to the contributor guidelines

shawnborton · 2021-05-14T16:37:16Z

src/pages/settings/AboutPage.js

+                    shouldShowRightArrow
+                />
+            ))}
+            <View style={[styles.sidebarFooter, styles.pAbsolute, styles.b0]}>


I think this would be better off if we used flexbox instead of absolute positioning. For example, what happens when someone's screen height is short?

I think we could accomplish this by using something like alignSelf: flex-end assuming this child would live within a flexbox parent that has flex-direction: column or something like that.

Align self was not working that’s why I went with absolute. Now I have migrated this flex based approach.

Great! It would be helpful to see this layout on web where the browser height is not very tall. Could you test there and let me know how it looks?

The setting page and all View based pages are breaking, it would be great if we wrap all of them inside ScrollView

Yeah I agree - ideally that page would scroll if it needs to.

Adding ScrollView 👍🏼

Done 🚀

Screen.Recording.2021-05-14.at.11.26.17.PM.mov

Nice! Looking pretty good, but I would think the "Read the terms..." should have some margin on top of it so it doesn't get too close to the menu options, and should be closer to the bottom edge of the view:

Awesome, looks great!

shawnborton · 2021-05-14T22:05:31Z

For the "Report a bug" row, this should not be opening a new window and should redirect to the 1:1 chat with Concierge within the Expensify.cash app itself.

pranshuchittora · 2021-05-14T22:07:33Z

For the "Report a bug" row, this should not be opening a new window and should redirect to the 1:1 chat with Concierge within the Expensify.cash app itself.

It is redirecting

src/pages/settings/AboutPage.js

pranshuchittora · 2021-05-15T10:21:28Z

Hi @shawnborton @Julesssss
Tested this feature for all the platforms and it is working as expected. Attaching the screen recordings in the PR description :)

Dal-Papa

Please undo all of the automatic (or not) style changes, it makes the PR difficult to review and doesn't comply with this section of our contributing guidelines.

src/languages/en.js

src/pages/settings/InitialPage.js

src/languages/en.js

Julesssss

Can you please remove all the unnecessary formatting changes? It's adding time to the review. Thanks!

src/pages/settings/AboutPage.js

src/pages/settings/AppDownloadLinks.js

src/pages/settings/AboutPage.js

Dal-Papa

My concerns are resolved. I'll let the other reviewers have the final approve.

src/CONST.js

src/pages/settings/AboutPage.js

Julesssss

There's one outstanding comment to resolve. Also, I was unable to click 'Report a bug', not sure why this isn't working, because I definitely have the concierge chat stored locally.

Screen.Recording.2021-05-18.at.10.35.16.mov

src/ROUTES.js

pranshuchittora · 2021-05-18T12:28:13Z

There's one outstanding comment to resolve. Also, I was unable to click 'Report a bug', not sure why this isn't working, because I definitely have the concierge chat stored locally.

@Julesssss looks like there is no inconsistency in the reportName

Can you please confirm, what are the possible values for this? Concierge or concierge@expensify.com

I have pushed the comment styling fix :)

src/pages/settings/AboutPage.js

Julesssss · 2021-05-18T12:49:41Z

Can you please confirm, what are the possible values for this? Concierge or concierge@expensify.com

Actually, I think we should use the alternative mentioned here: https://github.com/Expensify/Expensify.cash/pull/2926/files#r634344289

pranshuchittora · 2021-05-18T13:45:53Z

Can you please confirm, what are the possible values for this? Concierge or concierge@expensify.com

Actually, I think we should use the alternative mentioned here: https://github.com/Expensify/Expensify.cash/pull/2926/files#r634344289

I have made all the changes :)

cc @Julesssss

Julesssss · 2021-05-19T10:55:30Z

Hi @pranshuchittora, the PR looks good to me, but can you add test steps so that QA knows what to check. Thanks!

pranshuchittora · 2021-05-19T12:58:17Z

Hi @pranshuchittora, the PR looks good to me, but can you add test steps so that QA knows what to check. Thanks!

@Julesssss I have add the testing strategy for QA in the PR description

Julesssss

Tested well on all platforms.

pranshuchittora · 2021-05-19T13:51:22Z

Tested well on all platforms.

Thanks, @Julesssss for the awesome PR review. Apologies for the auto linting due to some bad configuration of my IDE.
Good Day

tgolen

@pranshuchittora I ran into some questions with some of the propType changes here.

tgolen · 2021-05-19T20:35:21Z

src/components/MenuItem.js

-    /** Boolean whether to display the ArrowRight icon */
-    shouldShowRightArrow: PropTypes.bool,
+    /** Boolean whether to display the right icon */
+    shouldShowRightIcon: PropTypes.bool,


Why do we need an extra prop for this? Shouldn't the render method just look at iconRight and if there is no value, then it wouldn't display anything?

The reason for having a boolean because for every usage we need to import the icon and pass that as a prop. I agree that we can do that but the majority of the use cases in the code uses an arrow icon, hence importing it every time might be tedious.

Yeah, my original suggestion was to simply check if the icon prop had a value.

I wouldn't block a suggestion to make this change, but upon seeing the PR it made sense to keep the arrow as default, because overriding the default icon is the exception to the rule here -- every other menu item will display the chevron and it felt redundant to specifically state this on every menu item.

tgolen · 2021-05-19T20:36:38Z

src/components/MenuItem.js

@@ -63,9 +68,9 @@ const MenuItem = ({
                        </Text>
                    </View>
                </View>
-                {shouldShowRightArrow && (
+                {shouldShowRightIcon && (


I see several places in the code which passes iconRight but not shouldShowRightIcon and so that would mean those places never have the iconRight displayed?

Example: https://github.com/Expensify/Expensify.cash/blob/bbdd6b6d6b83d7e980dc7f99ffbdab9e6f33100d/src/pages/settings/AppDownloadLinks.js#L21-L39

Pls have a look at the iterator over the data
https://github.com/Expensify/Expensify.cash/blob/bbdd6b6d6b83d7e980dc7f99ffbdab9e6f33100d/src/pages/settings/AppDownloadLinks.js#L58

Hmm, upon review it is a bit confusing that shouldShowRightIcon is defined outside of the menuItems array

This is because all these items share the same icon property so it’s better to define it once.

In that case, the argument against repeating the Right Icon import is a bit weaker, because it would also be defined once for each page...

Stepping back, I think there are three cases trying to be handled here:

There is no right icon

There is a right icon, and it defaults to an arrow

There is a right icon, and it is a specific icon

Rather than trying to tackle this in a single component, I think it would be much better to separate this into two components:

<MenuItem> has no right icon

<MenuItemWithRightIcon> has a right icon that defaults to the arrow

<MenuItemWithRightIcon icon={foo}> has a right icon that is a specific icon

I have not looked at all the places in the code where we currently use <MenuItem> so I don't know how complex it would be to implement, but I think having the two separate components makes it much easier for someone else to come along and use them without needing to dig into the source to figure out how the props work.

@tgolen I would rather prefer this

<MenuItem> <MenuItem.RightIcon icon={foo} /> </MenuItem>

or

<MenuItem> <MenuItem.RightIcon> <Icon /> // or <SomeComposedComponent /> <MenuItem.RightIcon> </MenuItem>

I really agree that refactoring of building blocks (components) is required to support long term goals.
If we want to move forward with this or any other API for components then I can pull this off as I have some significant experience in design systems.

I am one of the core maintainer of React Native Elements

cc @Julesssss

@pranshuchittora Ah yes, those would work as well! Can you help me understand why you would prefer that pattern over the one I proposed? Is it just personal preference, or is there a specific reason for it?

It’s a bit opinionated but makes things easy to maintain and also makes the code readable (from consumer end)
Ref -> https://react-bootstrap.github.io/components/cards/#basic-example

tgolen · 2021-05-19T20:37:19Z

src/components/MenuItem.js


    /** A boolean flag that gives the icon a green fill if true */
    success: PropTypes.bool,
+
+    // Overrides the icon for shouldShowRightIcon
+    iconRight: PropTypes.element,


I'm getting a propType warning from this... been trying to track it down but not having any luck. Maybe you know?

I think this should be changed

- iconRight: PropTypes.element + iconRight: PropTypes.elementType

@pranshuchittora would you mind holding on until @tgolen is able to reply, then create a new PR with these suggestions? Thanks!

tgolen · 2021-05-21T18:09:39Z

Ah yes, I can see that being valuable in the example of implementing that into an entire component library. I think since we are not following any of those patterns in our components, that we don't really want to start now (without re-thinking about how ALL our components are implemented). Because of that, I'd prefer the method that I suggested for now. Thanks for the extra context!

…

On Thu, May 20, 2021 at 11:41 PM Pranshu Chittora ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/components/MenuItem.js <#2926 (comment)> : > @@ -63,9 +68,9 @@ const MenuItem = ({ </Text> </View> </View> - {shouldShowRightArrow && ( + {shouldShowRightIcon && ( It’s a bit opinionated but makes things easy to maintain and also makes the code readable (from consumer end) Ref -> https://react-bootstrap.github.io/components/cards/#basic-example — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2926 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB2TIBECD3MYDBBXCI3TOXXBXANCNFSM444MNGKQ> .

eVoloshchak · 2022-11-10T12:57:43Z

Dropping a note that this has caused a 'regression': #12238
Not sure if it can be called a regression, probably wasn't in the requirements, but when using onPress to open links instead of passing href prop, links are not highlighted when hovering over

pranshuchittora requested a review from a team as a code owner May 14, 2021 12:54

MelvinBot requested review from Dal-Papa and removed request for a team May 14, 2021 12:55

pranshuchittora force-pushed the pranshuchittora/fix/2811 branch from 58c873b to 5515545 Compare May 14, 2021 15:31

shawnborton reviewed May 14, 2021

View reviewed changes

pranshuchittora commented May 14, 2021

View reviewed changes

src/pages/settings/AboutPage.js Outdated Show resolved Hide resolved

Dal-Papa suggested changes May 17, 2021

View reviewed changes

Julesssss reviewed May 17, 2021

View reviewed changes

src/languages/en.js Outdated Show resolved Hide resolved

Julesssss reviewed May 17, 2021

View reviewed changes

src/languages/en.js Outdated Show resolved Hide resolved

Julesssss reviewed May 17, 2021

View reviewed changes

src/languages/en.js Outdated Show resolved Hide resolved

Julesssss requested changes May 17, 2021

View reviewed changes

Julesssss reviewed May 17, 2021

View reviewed changes

src/pages/settings/AboutPage.js Outdated Show resolved Hide resolved

Julesssss reviewed May 17, 2021

View reviewed changes

src/pages/settings/AppDownloadLinks.js Outdated Show resolved Hide resolved

pranshuchittora added 2 commits May 17, 2021 15:39

chore: PR review changes

2f31a92

chore: PR review changes

bbdd6b6

pranshuchittora force-pushed the pranshuchittora/fix/2811 branch from d64977c to bbdd6b6 Compare May 17, 2021 10:12

fix: Remove hooks usage

c171c13

pranshuchittora commented May 17, 2021

View reviewed changes

src/pages/settings/AboutPage.js Outdated Show resolved Hide resolved

Dal-Papa previously approved these changes May 17, 2021

View reviewed changes

Julesssss reviewed May 17, 2021

View reviewed changes

src/CONST.js Outdated Show resolved Hide resolved

Julesssss reviewed May 17, 2021

View reviewed changes

src/pages/settings/AboutPage.js Outdated Show resolved Hide resolved

fix: Minor PR review changes

e5cd2c4

pranshuchittora dismissed Dal-Papa’s stale review via e5cd2c4 May 17, 2021 17:00

pranshuchittora requested a review from Julesssss May 17, 2021 17:01

Julesssss requested changes May 18, 2021

View reviewed changes

parasharrajat reviewed May 18, 2021

View reviewed changes

src/ROUTES.js Outdated Show resolved Hide resolved

chore: Minor fixes

15bd2f2

Julesssss reviewed May 18, 2021

View reviewed changes

src/pages/settings/AboutPage.js Outdated Show resolved Hide resolved

fix: Report a bug redirection logic change

94c32e7

pranshuchittora requested a review from Julesssss May 18, 2021 13:47

Julesssss approved these changes May 19, 2021

View reviewed changes

Julesssss merged commit a97df32 into Expensify:main May 19, 2021

roryabraham mentioned this pull request May 19, 2021

Deploy Checklist: Expensify.cash 2021-05-19 #3000

Closed

10 tasks

tgolen reviewed May 19, 2021

View reviewed changes

eVoloshchak mentioned this pull request Nov 10, 2022

[HOLD for payment 2022-11-16] [$250] URL address popup won’t show when hovered on terms/privacy links on about page reported by @huzaifa-99 #12238

Closed

feat: About Page #2926

feat: About Page #2926

Conversation

pranshuchittora commented May 14, 2021 • edited by Julesssss Loading

Details

Fixed Issues

QA Steps / Testing Strategy 🧪

Tested On

Screenshots

iOS

Android

Desktop App

Web

github-actions bot commented May 14, 2021 • edited Loading

pranshuchittora commented May 14, 2021 • edited Loading

pranshuchittora commented May 14, 2021

Julesssss commented May 14, 2021 • edited Loading

pranshuchittora commented May 14, 2021

Julesssss commented May 14, 2021

pranshuchittora commented May 14, 2021

Julesssss commented May 14, 2021

pranshuchittora commented May 14, 2021

pranshuchittora commented May 14, 2021

pranshuchittora commented May 14, 2021

shawnborton May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Julesssss commented May 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawnborton commented May 14, 2021

pranshuchittora commented May 14, 2021

pranshuchittora commented May 15, 2021

Dal-Papa left a comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

Dal-Papa left a comment

Choose a reason for hiding this comment

Julesssss left a comment

Choose a reason for hiding this comment

pranshuchittora commented May 18, 2021 • edited Loading

Julesssss commented May 18, 2021

pranshuchittora commented May 18, 2021 • edited Loading

Julesssss commented May 19, 2021

pranshuchittora commented May 19, 2021

Julesssss left a comment

Choose a reason for hiding this comment

pranshuchittora commented May 19, 2021

tgolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Julesssss May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranshuchittora commented May 14, 2021 •

edited by Julesssss

Loading

github-actions bot commented May 14, 2021 •

edited

Loading

pranshuchittora commented May 14, 2021 •

edited

Loading

Julesssss commented May 14, 2021 •

edited

Loading

shawnborton May 14, 2021 •

edited

Loading

pranshuchittora commented May 18, 2021 •

edited

Loading

pranshuchittora commented May 18, 2021 •

edited

Loading

pranshuchittora May 20, 2021 •

edited

Loading

Julesssss May 20, 2021 •

edited

Loading

pranshuchittora May 20, 2021 •

edited

Loading