[waiting for payment][$250] Share code- QR code expands to full size after opening for WS rooms, tasks and threads

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: 9.0.2-0 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4675372 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

1. Open the app and log in
2. Navigate to any task, thread or a workspace room
3. Tap the header and select Share

Expected Result:

The QR code is displayed fully

Actual Result:

The QR code is displayed small briefly and expands to the full size

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• [x] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [ ] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/91c91457-a976-4e4d-a368-05544fcacbb0

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e0a4dbc404a8ab34
• Upwork Job ID: 1805949354249394574
• Last Price Increase: 2024-07-10
• Automatic offers:
• alitoshmatov | Reviewer | 103091801
• huult | Contributor | 103091804

Issue Owner

Current Issue Owner: @adelekennedy

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot[bot]]

Triggered auto assignment to @danieldoglas (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

[github-actions[bot]]

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[izarutskaya]

@adelekennedy I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

[izarutskaya]

We think this issue might be related to the #vip-vsb

[huult]

Proposal

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

QRCode change size after layout change

What is the root cause of that problem?

Default QR code size is 120px, after layout change the QR code recalculate size at this line

https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/components/QRShare/index.tsx#L32

so the UI QRCode will update width and height

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

We have to set the default initial height for qrcode size at this line base on platform

https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/components/QRShare/index.tsx#L19

ST like that

+    const isMobilePlatform = Platform.OS === 'android' || Platform.OS === 'ios';
+    const isMobileBrowser = Browser.isMobile();
+    const isMobile = isMobilePlatform || isMobileBrowser;
+    const { windowWidth } = useWindowDimensions();
+.   const qrShareContainerWidth = isMobile ? windowWidth : variables.sideBarWidth;

-     const [qrCodeSize, setQrCodeSize] = useState<number | undefined>();
+    // styles.ph5.paddingHorizontal * 2: This is the padding outside of the QR code.
+    // variables.qrShareHorizontalPadding * 2: This is the padding inside of the QR code.
+    // default size is remaining width after minus padding from parent container
+    const [qrCodeSize, setQrCodeSize] = useState<number>(
+        qrShareContainerWidth - (styles.ph5.paddingHorizontal * 2) - (variables.qrShareHorizontalPadding * 2)
+    );

POC https://github.com/Expensify/App/assets/20178761/9856157d-14d2-4d24-9077-16e8cfc5fb51

What alternative solutions did you explore? (Optional)

[Sparth19]

Proposal

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

• The QR code is initially displayed briefly in a small size before expanding to full size.

What is the root cause of that problem?

• The initial value of qrCodeSize is undefined, so it defaults to 120.

• It is displayed at this size initially, and then adjusts based on onLayout changes. https://github.com/Expensify/App/blob/0c20881a308244867119efa6196481b65bb81274/src/components/QRShare/index.tsx#L30-L33

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

• we can use a Loader While onLayout is Calculating the Size.

• This will ensure that the QR code is displayed at the correct size from the beginning and avoids any brief display of an incorrectly sized QR code or resizing issues in any device sizes.

     {qrCodeSize ? (
          <QRCode
              getRef={(svg) => (svgRef.current = svg)}
              url={url}
              logo={logo}
              size={qrCodeSize}
              logoRatio={logoRatio}
              logoMarginRatio={logoMarginRatio}
          />
      ) : (
          <ActivityIndicator
              color={theme.spinner}
              size="large"
          />
      )}

What alternative solutions did you explore? (Optional)

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

@danieldoglas, @alitoshmatov, @adelekennedy Huh... This is 4 days overdue. Who can take care of this?

[alitoshmatov]

Thank you for your proposal @huult. This issue is also happening on ios app. So I don't think writing platform specific code is a good idea

[alitoshmatov]

Thank you for your proposal @Sparth19 , QR codes are readily available data showing loader just for fixing its layout seems a workaround

[huult]

@alitoshmatov My proposal is not focused on Android only, so I wrote a note below the code: ! we have to separate to specific platform and calculate based on that. My proposal covers both Android and iOS by applying index.native.ts because mobile uses a fixed size, while the web uses the responsive size

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[huult]

Proposal updated

Thank you for your proposal @huult. This issue is also happening on ios app. So I don't think writing platform specific code is a good idea

Hi @alitoshmatov , I have updated some proposals and explained why we used them. Can you check it again? Thank you.

For mobile devices, the screen width is fixed. We can get the device width using Dimensions.get('screen').width, and then calculate the QR code size based on this container's padding.

For other devices, we use a fixed sidebar width of 375px, and we can calculate the initial QR code size based on this width

Here is test branch

[alitoshmatov]

Most of the time onLayout function working in an instant, I guess it is an issue only in slower devices.

@truph01 We cannot use Dimensions.get directly, there is a useWindowDimensions hook for that. For your solution to be viable, we should prove that that window width is always equal to event.nativeEvent.layout.width to prevent size changes

[huult]

Proposal Updated

@truph01 We cannot use Dimensions.get directly, there is a useWindowDimensions hook for that. For your solution to be viable, we should prove that that window width is always equal to event.nativeEvent.layout.width to prevent size changes

@alitoshmatov I used the useWindowDimensions hook instead of directly using Dimensions.get to prevent size changes. Thank you.

Here is test branch

[dominictb]

Proposal

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

• The QR code is displayed small briefly and expands to the full size

What is the root cause of that problem?

• TheQRCode's size based on its parent View component: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/components/QRShare/index.tsx#L36-L38

• When the onLayout of the View component is called, we get the QRCode's size. When the onLayout is not called, QRShare's size is 120 by default.

• That leads to the behavior: The QR code is displayed small briefly and expands to the full size.

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

• Actually, we can get the QRCode's size directly instead of waiting until the onLayout is called.

• For example, in case: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/pages/ShareCodePage.tsx#L75-L92 we can know the QRCode's size is screenWidth - themeStyles.ph5.paddingHorizontal * 2 - variables.qrShareHorizontalPadding * 2. themeStyles.ph5.paddingHorizontal * 2 is the padding value we use in here: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/pages/ShareCodePage.tsx#L75

• Based on the above fact, I propose a solution:

  1. Update: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/components/QRShare/index.tsx#L15-L19 to:

     
     function QRShare({url, title, subtitle, logo, logoRatio, logoMarginRatio, initialQrCodeSize = 120}: QRShareProps, ref: ForwardedRef<QRShareHandle>) {
     const styles = useThemeStyles();
     const theme = useTheme();

  const [qrCodeSize, setQrCodeSize] = useState<number | undefined>(initialQrCodeSize);

2. Then, when we use QRShare, directly pass the initialQrCodeSize value. For example, this: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/pages/ShareCodePage.tsx#L83 should be:

                   <QRShare
                       initialQrCodeSize={initialQrCodeSize}

   with:

   const {isSmallScreenWidth} = useResponsiveLayout();
   const {windowWidth} = useWindowDimensions();
   const initialQrCodeSize = (isSmallScreenWidth ? windowWidth : variables.sideBarWidth) - themeStyles.ph5.paddingHorizontal * 2 - variables.qrShareHorizontalPadding * 2;

and this: https://github.com/Expensify/App/blob/224f09edbb511bd179d79c55f646d5522c1f955d/src/pages/workspace/WorkspaceProfileSharePage.tsx#L59 should be:

                    <QRShare
                        initialQrCodeSize={initialQrCodeSize}

with:

    const {isSmallScreenWidth} = useResponsiveLayout();
    const {windowWidth} = useWindowDimensions();
    const initialQrCodeSize = (isSmallScreenWidth ? windowWidth : variables.sideBarWidth) - themeStyles.ph9.paddingHorizontal * 2 - variables.qrShareHorizontalPadding * 2;

Note:

• The QRCode's size is not a constant among the screens, for example, in ShareCodePage, its parent has paddingHorizontal: 20 but in WorkspaceProfileSharePage that value is 36.

[huult]

@alitoshmatov I think this proposal duplicates my proposal. It still uses the initial container width to calculate the QR size after removing padding.

[dominictb]

@alitoshmatov My proposal ensures the QRCode size is dynamically determined based on its container. If we decide to fix the QRCode size in the future, how should we handle scenarios where the QRShare needs to adapt to different View component widths?

[danieldoglas]

friendly bump @alitoshmatov

[melvin-bot[bot]]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[melvin-bot[bot]]

@danieldoglas @alitoshmatov @adelekennedy this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot[bot]]

@danieldoglas, @alitoshmatov, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[alitoshmatov]

@huult Although @dominictb 's proposal looks similar to yours(even maybe inspired by yours), I do think they made a much needed improvement here. Calculating and passing qr code width in parent container and using isSmallScreenWidth rather then platform specific code is substantial difference in my opinion.

[truph01]

@alitoshmatov Sorry, any mistake here? I see you tagged me in this issue.

[alitoshmatov]

Sorry for inconvenience, @truph01 Right, I didn't mean to. Yes you can ignore this

[alitoshmatov]

I think we can go with @dominictb 's proposal which is similar to or even derived from @huult 's proposal.

I am not sure about my judgement here, if we look on each proposal separately, @dominictb 's proposal would definitely be the one. But considering @huult was working on their proposal and giving much needed direction I am in doubt here. Would love to hear your take on this one @danieldoglas

C+ reviewed 🎀 👀 🎀

[melvin-bot[bot]]

Current assignee @danieldoglas is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

[huult]

@alitoshmatov @danieldoglas My proposal will be provided as soon as possible and will still cover the issue. RegardingisSmallScreenWidth orisMobile, these are condition checks to detect the container if they want to cover the screen size of other devices, which can be updated during code review or while creating a pull request. As I checked the contributor rules and found item 6 of proposal for the job then other proposal should diferent MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable. If other proposal not used inital container with after remove padding to resolve this issue then it's ok.

[danieldoglas]

OK, I think we'll go with @huult's proposal. I agree that there are some differences between them, but I think @dominictb's proposal derived from the @huult's and we can address the smaller changes in the PR.

[melvin-bot[bot]]

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

[melvin-bot[bot]]

📣 @huult 🎉 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 📖

[dominictb]

@danieldoglas

• I don't think my proposal derived from the selected proposal. With my proposal, we can manually apply the size for the QRCode component, for example, if we want to use QRCode with size 100px, we can use <QRCode size={100} ... />.

• I bet that the factor that makes you consider that my proposal derived from the other one is the logic to get the size prop:

  const {isSmallScreenWidth} = useResponsiveLayout();
  const {windowWidth} = useWindowDimensions();
  const initialQrCodeSize = (isSmallScreenWidth ? windowWidth : variables.sideBarWidth) - themeStyles.ph9.paddingHorizontal * 2 - variables.qrShareHorizontalPadding * 2;

  It is because currently, we always use the QRCode component with the width sizeBarWidth minus any padding horizontal value. But in the future, we cannot make sure we will not use something like <QRCode size={100} ... >

[danieldoglas]

Assigning @mountiny to see the end of this while I'm OOO

[mountiny]

@dominictb I understand your concern, and I am sorry that this time, it did not work out. I think both of you and your solutions will work, and they are similar, so @huult should go ahead, as their proposal came in first.

@huult what is your ETA for the PR?

[huult]

I will create the pull requests soon. The ETA is the next day. Thank you.

[dominictb]

@mountiny @danieldoglas Thanks for your answer

[huult]

here is my PR

[huult]

Hi @mountiny , @danieldoglas , My PR was merged to production a week ago, but I noticed the Mevin bot isn't working as expected compared to other tickets I've worked on. Can I ask if this ticket is currently able to process a payment? Thank you!

[adelekennedy]

ty @danieldoglas let me handle payment now:

Payouts due:

• [x] Contributor: $250 @huult
• [x] Contributor+: $250 @alitoshmatov

Upwork job is here.

[adelekennedy]

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

• [ ] The PR that introduced the bug has been identified. Link to the PR:
• [ ] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [ ] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [ ] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[adelekennedy]

@alitoshmatov checklist above when you get a chance!

[alitoshmatov]

• The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/32690
• The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/32690/files#r1712561692
• A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
• Determine if we should create a regression test for this bug.

Regression Test Proposal

1. Open the app and log in.
2. Navigate to any task, thread, or workspace room.
3. Tap the header to open the details and select "Share".
4. Verify that the QR code is displayed in full size and not changing its size while page is opening.

Do we agree 👍 or 👎

cc: @adelekennedy