@swm/global nav menu v2 by adamgrzybowski · Pull Request #29104 · Expensify/App

adamgrzybowski · 2023-10-09T16:45:44Z

Details

WIP
The description will be updated.

Fixed Issues

$
PROPOSAL:

Tests

Verify that no errors appear in the JS console

Offline tests

QA Steps

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  - Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2023-10-13T01:23:47Z

@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot · 2023-10-13T01:56:46Z

@ Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

hayata-suenaga · 2023-10-13T01:59:36Z

Getting help from C+ here for review

hayata-suenaga · 2023-10-13T02:45:43Z

src/components/IFrame.js

+            return;
+        }
+
+        setOldDotURL(`${BASE_IFRAME_URL}/${newOldDotURL}`);


When the user clicks a link that navigates to a different OldDot page inside the iframe:

A link is clicked

OldDot inside the iframe will navigate to a different page

NewDot detect the URL change inside iframe

The callback is triggered and a navigation happens inside NewDot

routeName and params change

The URL of the iframe is set to the URL to which the embedded OldDot has already navigated to

So to avid the step #6, we can check if the URL of the iframe is the same as the one mapped to the routeName before setting oldDotURL

I actually don't know if resetting the src of the iframe to same URL will cause the iframe to reload. If it doesn't, then we can keep the current code

I did some testing and it turns out that we need to remount the iframe anyway. Keeping the iframe and navigating inside it will interfere with the browser history.

I did it in a way to remount as little as possible.

Maybe there is a way to prevent iframe from interfering with the browser history without remounting but I can't see anything at this moment. That's probably something worth investigating in the future

src/components/IFrame.js

src/languages/es.ts

src/SCREENS.ts

src/ROUTES.ts

src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js

src/pages/home/sidebar/GlobalNavigation/index.js

src/components/IFrame.js

src/languages/en.ts

src/languages/es.ts

src/libs/Navigation/AppNavigator/IframeTest.js

src/libs/Navigation/getTopMostCentralPaneRouteName.js

shawnborton · 2023-10-16T15:09:48Z

Can you make sure each sub nav item uses this spacing/structure?

The icon box should be 32x32, with a 20x20 icon centered in the middle:

The gap between icon and text should be 12px:

adamgrzybowski · 2023-10-23T15:05:17Z

I also realized that the workspaces/<workspace_id>/overview route is not working. Could you check this one please 🙇

Okay, so the URL object is encoding unsafe characters like { } etc. As a result, our output URL differs from the one expected by expensify.com

I am assuming that we are not going to change the expenisfy.com urls so I am going to look for the best way to work with the URLs while keeping the unsafe characters.

It feels like just concatenating strings may be an easy way to generate more bugs 😄

@hayata-suenaga

hayata-suenaga · 2023-10-24T01:40:01Z

@adamgrzybowski Thank you for investigating this 🙇

Okay, so the URL object is encoding unsafe characters like { } etc. As a result, our output URL differs from the one expected by expensify.com

So the URL we get inside the callback for the message event is actually encoded URL?

adamgrzybowski · 2023-10-24T08:27:30Z

@adamgrzybowski Thank you for investigating this 🙇

Okay, so the URL object is encoding unsafe characters like { } etc. As a result, our output URL differs from the one expected by expensify.com

So the URL we get inside the callback for the message event is actually encoded URL?

I don't know what is inside the callback because I can't get olddot running inside the iframe. But I am assuming this is working fine.

The encoded URL happens when we are generating a URL for the iframe. In the function addNewdotParams

@hayata-suenaga

hayata-suenaga · 2023-10-25T00:59:26Z

ah this one

function addNewDotParams(url) {
    const urlObj = new URL(url);
    urlObj.searchParams.append('isInNewDot', true);
    urlObj.searchParams.append('isDarkMode', true);
    return urlObj.toString();
}

@adamgrzybowski were you able to find a solution for this issue 😄 ?

adamgrzybowski · 2023-10-25T15:18:19Z

@hayata-suenaga As I mentioned in the update, I added decodeURIComponent to reverse encoding to safe form.

function addNewDotParams(url) {
    const urlObj = new URL(url);
    urlObj.searchParams.append('isInNewDot', true);
    urlObj.searchParams.append('isDarkMode', true);
    return decodeURIComponent(urlObj.toString());
}

hayata-suenaga · 2023-11-14T23:05:01Z

is this ready for review?

adamgrzybowski · 2023-11-16T09:21:33Z

@hayata-suenaga Haven't we removed the global nav? I am not sure what should happen with this PR

hayata-suenaga · 2023-11-16T16:43:44Z

ah sorry I thought @wojciech.boman is working on this PR. I asked him to finish this PR (but removing code related to global nav while keeping the code related to the logic for navigation synchronization)

stitesExpensify · 2023-12-14T15:16:30Z

Should this PR be closed? Or do we still want to keep some of it?

hayata-suenaga · 2023-12-14T17:09:08Z

I think we can close this PR now but this PR has a navigation sync logic between NewDot and OldDot contained. I don't think we're embedding any OldDot pages anytime soon, so I think we can close this and then reopen it or copy part of the code if necessary in the future.

akinwale · 2023-12-14T17:20:20Z

@hayata-suenaga Would there be any compensation for the review work done here?

hayata-suenaga · 2023-12-15T17:53:04Z

Payment summary

@akinwale did a CP preview as a C+ for this closed PR. The standard C+ review compensation should be applied here.

@akinwale I pinged an internal employee for payment.

adamgrzybowski added 6 commits October 9, 2023 11:02

initial

9ef6f2e

add icons

8ad3d95

center text for global nav items

8d0b65b

styles and consts for subnavigation

753bd94

fix typo

a7fc762

integrate iframe into customStackNavigator

6d2f27e

hayata-suenaga requested review from hayata-suenaga and stitesExpensify October 12, 2023 23:52

hayata-suenaga added the Design label Oct 12, 2023

hayata-suenaga marked this pull request as ready for review October 13, 2023 01:23

hayata-suenaga requested a review from a team as a code owner October 13, 2023 01:23

melvin-bot bot removed the request for review from a team October 13, 2023 01:23

hayata-suenaga requested a review from a team October 13, 2023 01:56

melvin-bot bot removed the request for review from a team October 13, 2023 01:56

change name to individual_workspace

76f9ae4

hayata-suenaga reviewed Oct 13, 2023

View reviewed changes

akinwale reviewed Oct 13, 2023

View reviewed changes

adamgrzybowski added 8 commits October 13, 2023 15:30

use useMemo in createCustomStackNavigator

566a6ca

fix comments

ec1f327

use last in getTopMostCentralPaneRoute

fbf2af8

remove IframeTest

1b39f1e

use existing translation for workspace

c51495e

remove title from options for centralPaneNavigator

2134a31

adjust links in iframe screen

4edfeb1

Merge branch 'main' into @swm/global-nav-menu-v2

1ebab67

hayata-suenaga mentioned this pull request Oct 17, 2023

Feat/placeholder web #29092

Closed

59 tasks

adamgrzybowski added 2 commits October 23, 2023 13:38

adjust iframe

f0205cf

Merge branch 'main' into @swm/global-nav-menu-v2

09a2edd

adamgrzybowski added 2 commits October 23, 2023 17:38

use decode uri component for olddot urls

dadac31

Merge branch 'main' into @swm/global-nav-menu-v2

7869f8c

adamgrzybowski added 6 commits October 30, 2023 19:26

Merge branch 'main' into @swm/global-nav-menu-v2

5596354

Merge branch 'main' into @swm/global-nav-menu-v2

662018a

fix bigger screen blocking view

f43f741

fix imports

b1cbad1

change base url for iframe

6bf0d44

use replace instead of changing src

da480e3

WojtekBoman requested a review from a team as a code owner November 10, 2023 11:36

Merge branch 'main' into @swm/global-nav-menu-v2

d2688b7

coleaeason removed the request for review from a team November 17, 2023 00:23

Merge branch 'main' into @swm/global-nav-menu-v2

3819ac1

hayata-suenaga closed this Dec 14, 2023

hayata-suenaga added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Dec 15, 2023

hayata-suenaga mentioned this pull request Dec 20, 2023

[$500] Handle payment for PR #29104 #33368

Closed

Conversation

adamgrzybowski commented Oct 9, 2023 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

melvin-bot bot commented Oct 13, 2023

Uh oh!

melvin-bot bot commented Oct 13, 2023

Uh oh!

hayata-suenaga commented Oct 13, 2023

Uh oh!

hayata-suenaga Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

adamgrzybowski Oct 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shawnborton commented Oct 16, 2023

Uh oh!

adamgrzybowski commented Oct 23, 2023

Uh oh!

hayata-suenaga commented Oct 24, 2023

Uh oh!

adamgrzybowski commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hayata-suenaga commented Oct 25, 2023

Uh oh!

adamgrzybowski commented Oct 25, 2023

Uh oh!

hayata-suenaga commented Nov 14, 2023

Uh oh!

adamgrzybowski commented Nov 16, 2023

Uh oh!

hayata-suenaga commented Nov 16, 2023

Uh oh!

stitesExpensify commented Dec 14, 2023

Uh oh!

hayata-suenaga commented Dec 14, 2023

Uh oh!

akinwale commented Dec 14, 2023

Uh oh!

hayata-suenaga commented Dec 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Payment summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

adamgrzybowski commented Oct 9, 2023 •

edited by github-actions bot

Loading

adamgrzybowski commented Oct 24, 2023 •

edited

Loading

hayata-suenaga commented Dec 15, 2023 •

edited

Loading