search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.34k stars 2.77k forks source link

[$1000] Create a useResponsiveLayout hook #30528

Open roryabraham opened 10 months ago

roryabraham commented 10 months ago

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
melvin-bot[bot] commented 10 months ago

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

melvin-bot[bot] commented 10 months ago

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] commented 10 months ago

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

alitoshmatov commented 10 months ago

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 commented 10 months ago

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',
              });
            }
          }
        }
      },
    };
  },
};
Screenshot 2023-10-27 at 22 12 57

What alternative solutions did you explore? (Optional)

NA

rayane-djouah commented 10 months ago

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
/>
  1. 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,
        };
    }
}
  1. 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();
  1. 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 commented 10 months ago

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] commented 10 months ago

📣 @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. Screen Shot 2022-11-16 at 4 42 54 PM Format: 
    Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
roryabraham commented 10 months ago

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 commented 10 months ago

Thank you, I will raise a PR asap

getusha commented 9 months ago

@rayane-djouah any updates?

rayane-djouah commented 9 months ago

Updating now

maddylewis commented 9 months ago

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

rayane-djouah commented 9 months ago

PR still in review by C+

roryabraham commented 9 months ago

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

rayane-djouah commented 9 months ago

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 introduce a new lint rule that prohibits using isSmallScreenWidth from useWindowDimensions, advising developers to use shouldUseNarrowLayout instead. https://github.com/Expensify/eslint-config-expensify/pull/82
  11. A PR to Introduce the new lint rule advising developers to preferably use shouldUseNarrowLayout over isSmallScreenWidth from useResponsiveLayout, with a warning level. https://github.com/Expensify/eslint-config-expensify/pull/116
  12. A PR to upgrade the version of eslint in App to use the new lint rule. We might also include necessary changes after introducing the rule
melvin-bot[bot] commented 8 months ago

Upwork job price has been updated to $1000

roryabraham commented 8 months ago

Doubling the bounty here due to large scope

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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:

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.

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

melvin-bot[bot] commented 8 months ago

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:

maddylewis commented 8 months ago

moving to daily since payment is coming up soon

getusha commented 8 months ago

@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 commented 8 months ago

Agreed. Left some comments on the 2nd PR.

roryabraham commented 8 months ago

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

rayane-djouah commented 8 months ago

sorry for the delay, I updated the PR now

melvin-bot[bot] commented 8 months ago

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] commented 8 months ago

Payment Summary

Upwork Job

BugZero Checklist (@alexpensify)

alexpensify commented 8 months ago

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] commented 8 months ago

⚠️ 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.

alexpensify commented 8 months ago

@getusha two requests:

  1. Please apply to the Upwork job: https://www.upwork.com/jobs/~0123a7c663aaab8163

  2. Can we get feedback if this Deploy Blocker notice is accurate to the first PR or unrelated to this GH? Thanks!

melvin-bot[bot] commented 8 months ago

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

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

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

melvin-bot[bot] commented 8 months ago

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:

rayane-djouah commented 8 months ago

I updated the approach here cc @getusha @roryabraham

alexpensify commented 8 months ago

@getusha and @roryabraham - to confirm, this one is pending one last PR, so the January 24 payment date is inaccurate - right? Thanks!

roryabraham commented 8 months ago

Yes, there's more than 1 outstanding PR. This should not be awaiting payment yet.

@getusha any update on when you'll be able to review this? It's not particularly high-priority, but better to not have it drag on longer than necessary. If you're currently prioritizing something else, that might be ok – just try to communicate a bit more actively. Thanks!

alexpensify commented 8 months ago

Thank you for confirming!

getusha commented 8 months ago

@alexpensify here is a list of PRs we should complete before payment https://github.com/Expensify/App/issues/30528#issuecomment-1868485991

@rayane-djouah @roryabraham i think we should avoid putting the issue link on upcoming PRs to make the automation stop triggering.

alexpensify commented 7 months ago

Thank you for this update and noted!

getusha commented 7 months ago

@rayane-djouah when do you think you'll be able to raise the next PR?

rayane-djouah commented 7 months ago

PR raised @getusha

getusha commented 7 months ago

Thanks, will try to complete the review tomorrow.

roryabraham commented 7 months ago

@rayane-djouah @getusha just checking in here – how are we doing and what are our next steps?

Looks like @rayane-djouah needs to update https://github.com/Expensify/App/pull/34837 to resolve some merge conflicts. Anything else?

getusha commented 7 months ago

needs to update https://github.com/Expensify/App/pull/34837 to resolve some merge conflicts. Anything else?

Yes, @rayane-djouah will complete the checklist & resolve conflicts.

rayane-djouah commented 7 months ago

updated the PRs list

alexpensify commented 7 months ago

Looks like we are making progress here!

alexpensify commented 6 months ago

Weekly Update: Making progress

alexpensify commented 6 months ago

Weekly Update: @rayane-djouah some of the PRs are still in draft mode

Can we get an update in the PR list you put together here - https://github.com/Expensify/App/issues/30528#issuecomment-1868485991

Thanks!

rayane-djouah commented 6 months ago

https://github.com/Expensify/App/pull/36292 still in review

alexpensify commented 6 months ago

The PR listed above is still in review