[HOLD for payment 2024-10-30] [$1000] Create a useResponsiveLayout hook

[roryabraham]

Slack context: https://expensify.slack.com/archives/C01GTK53T8Q/p1698364514493649

Problem

There are some times when we want to use the same layout on mobile devices or in the RHP on wide screens. In these cases, we can't rely on isSmallScreenWidth from useWindowDimensions to determine which layout to use, so we have hard-coded and/or different solutions we use to address the same problem.

Solution

1. In the navigation stack, add an isInRHP route prop to all screens in the RHP.

2. Write a custom hook called useResponsiveLayout that does something like this:

   function useResponsiveLayout() {
       const {isSmallScreenWidth} = useWindowDimensions();
       const {params} = useRoute();
       return {
           shouldUseNarrowLayout = isSmallScreenWidth || params.isInRHP,
       };
   }
   

3. Replace most layout-motivated uses of useWindowDimensions with useResponsiveLayout like so:

   - const {isSmallScreenWidth} = useWindowDimensions();
   + const {shouldUseNarrowLayout} = useResponsiveLayout();
   

4. Introduce a new lint rule that prohibits using isSmallScreenWidth from useWindowDimensions, advising developers to use shouldUseNarrowLayout instead.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f36dcb081cdf74b2
• Upwork Job ID: 1717949899365408768
• Last Price Increase: 2023-12-29

Issue Owner

Current Issue Owner: @alexpensify

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~01f36dcb081cdf74b2

[melvin-bot[bot]]

Triggered auto assignment to @maddylewis (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @getusha (External)

[alitoshmatov]

I can work on this if no one is assigned yet, or should I write proposal

Since it is a new feature main idea is explained well in description, and I think anything else could be discussed in the PR

[ZhenjaHorbach]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Create a useResponsiveLayout hook

What is the root cause of that problem?

This is a new functionality

What changes do you think we should make in order to solve the problem?

First, we need open RHP modals navigator And add new route option isInRHP in screenOptions

Then we need to create a new hook For example, using the suggested option

And then we need to find isSmallScreenWidth values Remove those values and use shouldUseNarrowLayout from the new hook

And at the last, we need to introduce a new lint rule that disallow using isSmallScreenWidth from useWindowDimensions

We can write a new rule

module.exports = {
  meta: {
    type: 'problem',
    docs: {
      description: 'Do not use isSmallScreenWidth from useWindowDimensions. Instead of that use shouldUseNarrowLayout from useResponsiveLayout',
      category: 'Best Practices',
    },
  },
  create: function (context) {
    return {
      VariableDeclarator(node) {
        if ( node.init.callee && node.init.callee.name === 'useWindowDimensions') {
          const properties = node.id.properties;
          for (const property of properties) {
            if (property.key.name === 'isSmallScreenWidth') {
              context.report({
                node,
                message: 'Do not use isSmallScreenWidth from useWindowDimensions. Instead of that use shouldUseNarrowLayout from useResponsiveLayout',
              });
            }
          }
        }
      },
    };
  },
};

What alternative solutions did you explore? (Optional)

NA

[rayane-djouah]

Proposal

Please re-state the problem that we are trying to solve in this issue.

The problem we aim to address is the lack of a consistent and reliable method to determine the appropriate layout for mobile devices and wide screens within our application. Currently, we rely on the isSmallScreenWidth property from the useWindowDimensions hook to make layout decisions. However, this approach falls short in situations where we need to maintain the same layout for both mobile devices and the Right-Hand Panel (RHP) on wide screens. As a result, we resort to hard-coded solutions or utilize different methods, which leads to code inconsistency and maintainability challenges.

What is the root cause of that problem?

The root cause of this problem lies in the absence of a dedicated mechanism to determine the layout based on specific conditions, such as whether a screen is within the RHP or on a small screen.

What changes do you think we should make in order to solve the problem?

1. Introduce isInRHP:

https://github.com/Expensify/App/blob/7b836cfcee79084177191e3dfb4a927698b09eb4/src/libs/Navigation/AppNavigator/ModalStackNavigators.tsx#L60-L64

Include an isInRHP option with a default value (true). This initial param will allow screens to determine whether they are within the Right-Hand Panel (RHP) and make layout decisions accordingly.

<ModalStackNavigator.Screen
    key={name}
    name={name}
    getComponent={(screens as Required<Screens>)[name as Screen]}
    initialParams={{isInRHP: true} as TStackParams[string]}  //here
/>

3. Create useResponsiveLayout Custom Hook: Develop a custom hook called useResponsiveLayout that provides a unified way to determine the appropriate layout. This hook will take advantage of useWindowDimensions and the isInRHP route prop to decide whether a narrow layout should be used. The hook's implementation is as follows:

export default function useResponsiveLayout(): ResponsiveLayoutResult {
    const {isSmallScreenWidth} = useWindowDimensions();
    try {
        const {params} = useRoute<RouteProp<RouteParams, 'params'>>();
        return {shouldUseNarrowLayout: isSmallScreenWidth || (params?.isInRHP ?? false)};
    } catch (error) {
        return {
            shouldUseNarrowLayout: isSmallScreenWidth,
        };
    }
}

4. Replace useWindowDimensions with useResponsiveLayout: Update the codebase to replace most instances where useWindowDimensions is used for layout decisions with useResponsiveLayout as follows:

Before:

const { isSmallScreenWidth } = useWindowDimensions();

After:

const { shouldUseNarrowLayout } = useResponsiveLayout();

5. Introduce a Lint Rule: Implement a new linting rule in https://github.com/Expensify/eslint-config-expensify/tree/main/eslint-plugin-expensify that prohibits the direct use of isSmallScreenWidth from useWindowDimensions for layout determination. This rule will encourage developers to use 'shouldUseNarrowLayout' from useResponsiveLayout instead, ensuring consistency in layout decisions throughout the codebase.

const _ = require('underscore');
const message = require('./CONST').MESSAGE.PREFER_USE_RESPONSIVE_FOR_LAYOUT;

module.exports = {
    meta: {
        type: 'problem',
        docs: {
            description: 'Prohibit the direct use of isSmallScreenWidth from useWindowDimensions',
        },
    },
    create(context) {
        return {
            VariableDeclarator(node) {
                if (!node.init || !node.init.callee || node.init.callee.name !== 'useWindowDimensions') {
                    return;
                }
                // Check for 'const {isSmallScreenWidth, ...} = useWindowDimensions();' pattern
                if (node.id.type === 'ObjectPattern') {
                    node.id.properties.forEach((property) => {
                        if (!property.key || property.key.name !== 'isSmallScreenWidth') {
                            return;
                        }
                        context.report({
                            node: property,
                            message,
                        });
                    });
                }

                // Check for 'const var = useWindowDimensions();' and use of this var
                const variableName = node.id.name;
                const variableUsages = _.filter(context.getScope().references, reference => reference.identifier.name === variableName);
                variableUsages.forEach((usage) => {
                    const parent = usage.identifier.parent;

                    // Check for 'const isSmallScreen = var.isSmallScreenWidth;' pattern
                    if (parent.type === 'MemberExpression' && parent.property.name === 'isSmallScreenWidth') {
                        context.report({
                            node: parent.property,
                            message,
                        });
                    }

                    // Check for 'const {isSmallScreenWidth} = var;' pattern
                    if (parent.type === 'VariableDeclarator' && parent.id.type === 'ObjectPattern') {
                        parent.id.properties.forEach((property) => {
                            if (!property.key || property.key.name !== 'isSmallScreenWidth') {
                                return;
                            }
                            context.report({
                                node: property,
                                message,
                            });
                        });
                    }
                });

            },
            MemberExpression(node) {
                // Check for 'const isSmallScreen = useWindowDimensions().isSmallScreenWidth;' pattern
                if (node.object.type !== 'CallExpression' || node.object.callee.name !== 'useWindowDimensions' || node.property.name !== 'isSmallScreenWidth') {
                    return;
                }
                context.report({
                    node,
                    message,
                });
            },
        };
    },
};

we should also add a test for this rule.

What alternative solutions did you explore? (Optional)

N/A

[WebflowGuru]

Problem: In the current application, there is a challenge when needing to use the same layout for both mobile devices and the right-hand panel (RHP) on wide screens. This problem arises because it's not always feasible to rely on the isSmallScreenWidth variable from useWindowDimensions to determine which layout to use. As a result, developers often resort to hard-coded and inconsistent solutions to address this issue.

Solution: To address this problem, the following changes are proposed:

Route Prop for RHP Detection: In the navigation stack, it is suggested to add an isInRHP route prop to all screens within the RHP. This addition allows for easy identification of screens within the RHP.

Custom Hook - useResponsiveLayout: Create a custom hook called useResponsiveLayout to determine the appropriate layout based on the screen width and the isInRHP route prop.

function useResponsiveLayout() {
    const { isSmallScreenWidth } = useWindowDimensions();
    const { params } = useRoute();
    return {
        shouldUseNarrowLayout: isSmallScreenWidth || params.isInRHP,
    };
}

Replace useWindowDimensions with useResponsiveLayout: To ensure a consistent approach to layout handling, replace most of the current uses of useWindowDimensions with useResponsiveLayout as shown below:

// Before
const { isSmallScreenWidth } = useWindowDimensions();

// After const { shouldUseNarrowLayout } = useResponsiveLayout(); Introduce a Lint Rule: To maintain code quality and consistency, introduce a new lint rule that discourages the use of isSmallScreenWidth from useWindowDimensions. Developers are advised to use shouldUseNarrowLayout instead. This proposed solution aims to provide a more unified and maintainable approach to handling responsive layouts in the application. These changes simplify the management of layouts across various screen sizes and ensure a more consistent user experience.

[melvin-bot[bot]]

📣 @WebflowGuru! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[roryabraham]

Thanks for all the proposals everyone. At this time I've decided to move forward with the one that feels the clearest and most high-quality, which in my opinion is from @rayane-djouah here.

[rayane-djouah]

Thank you, I will raise a PR asap

[getusha]

@rayane-djouah any updates?

[rayane-djouah]

Updating now

[maddylewis]

can someone provide an update on the status of this issue? thanks!

[rayane-djouah]

PR still in review by C+

[roryabraham]

Since we're splitting this up into a few PRs, @rayane-djouah, can you lay out the related PRs in sequence please?

[rayane-djouah]

Based on the changes in this PR https://github.com/Expensify/App/pull/31476, we're splitting this up into several PRs as follows:

1. A PR introducing useResponsiveLayout. https://github.com/Expensify/App/pull/33425
2. ~~A PR to add the isInRHP prop. https://github.com/Expensify/App/pull/33426~~ (I am modifying the logic for the useResponsiveLayout hook in https://github.com/Expensify/App/pull/34743. Instead of passing a parameter to screens in the modal stack navigator and checking for it in the hook, I am now obtaining the navigator name directly from the navigationRef in the hook without altering the navigation logic.)
3. A PR to replace usages of shouldShowSmallScreen and isInModal props in components. https://github.com/Expensify/App/pull/34837
4. A PR to replace usages of isSmallScreenWidth from useWindowDimensions in components folder files. https://github.com/Expensify/App/pull/36292
5. A PR to replace usages of isSmallScreenWidth from useWindowDimensions in pages folder files. https://github.com/Expensify/App/pull/43961
6. ~~A PR to replace usages of isSmallScreenWidth from useWindowDimensions in libs folder files. https://github.com/Expensify/App/pull/36294~~
7. ~~A PR to migrate remaining uses of isSmallScreenWidth prop from the withWindowDimensions HOC in functional components. https://github.com/Expensify/App/pull/36295~~
8. ~~A PR to rename isSmallScreenWidth parameter names in various functions (which doesn't have much effect). https://github.com/Expensify/App/pull/36296~~
9. A PR to replace remmaing usages of useWindowDimensions with useResponsiveLayout, refactor useWindowDimensions to export windowWidth and windowHeight only, and make useResponsiveLayout responsible for any logic determining the correct layout. https://github.com/Expensify/App/pull/46788
10. A PR to update eslint-config-expensify version to use the new rule after https://github.com/Expensify/eslint-config-expensify/pull/82 is merged.

[melvin-bot[bot]]

Upwork job price has been updated to $1000

[roryabraham]

Doubling the bounty here due to large scope

[melvin-bot[bot]]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot[bot]]

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.21-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

• https://github.com/Expensify/App/pull/33425

If no regressions arise, payment will be issued on 2024-01-11. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• [ ] External issue reporter
• [ ] Contributor that fixed the issue
• [ ] Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @getusha requires payment (Needs manual offer from BZ)
• @rayane-djouah requires payment (Needs manual offer from BZ)

[melvin-bot[bot]]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [ ] [@getusha] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [ ] [] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[maddylewis]

moving to daily since payment is coming up soon

[getusha]

@maddylewis since we are splitting the PR into multiple small PRs we should only process payment once the last PR gets merged. this is only the first one.

[roryabraham]

Agreed. Left some comments on the 2nd PR.

[roryabraham]

@rayane-djouah we're awaiting changes in the 2nd PR

[rayane-djouah]

sorry for the delay, I updated the PR now

[melvin-bot[bot]]

Issue is ready for payment but no BZ is assigned. @alexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[melvin-bot[bot]]

Payment Summary

Upwork Job

• ROLE: @getusha paid $1000 via Upwork
• ROLE: @rayane-djouah paid $1000 via Upwork

BugZero Checklist (@alexpensify)

• [x] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• [x] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1717949899365408768/hired)
• [x] I have paid out the Upwork contracts or cancelled the ones that are incorrect
• [x] I have verified the payment summary above is correct

[alexpensify]

Based on this comment here, this one is not ready for payment and there is an upcoming PR that needs to go to production before I can move forward with the payment process.

For now, I updated the Upwork job link:

https://www.upwork.com/jobs/~0123a7c663aaab8163

@getusha and @rayane-djouah -- please apply and we can be ready for the payment date.

Heads up, I will be offline until Tuesday, but will catch up then to identify the status of the last PR. I will not be actively watching over this GitHub during that period. If anything urgent is needed here, please ask for help in the Open Source Slack Room-- thanks!

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.