App icon indicating copy to clipboard operation
App copied to clipboard

[$250] Web - Search - Empty state page skeleton is cut off from the right

Open IuliiaHerets opened this issue 1 year ago β€’ 17 comments

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.68-0 Reproducible in staging?: Y Reproducible in production?: Y Issue was found when executing this PR: https://github.com/Expensify/App/pull/51913 Email or phone of affected tester (no customers): [email protected] Issue reported by: Applause Internal Team

Action Performed:

  1. Navigate to staging.new.expensify.com
  2. Navigate to Search
  3. Observe the background skeleton empty state on the right side

Expected Result:

The oval skeleton doesn't get cut off from the right

Actual Result:

The oval skeleton gets cut off from the right when there is a scroll bar on the right

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6679181_1732806399096!image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021865178692528155887
  • Upwork Job ID: 1865178692528155887
  • Last Price Increase: 2024-12-06
Issue OwnerCurrent Issue Owner: @

IuliiaHerets avatar Nov 28 '24 15:11 IuliiaHerets

Triggered auto assignment to @jliexpensify (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] avatar Nov 28 '24 15:11 melvin-bot[bot]

Proposal

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

The search page skeleton right side is cut off when there is a scrollbar.

What is the root cause of that problem?

This is how the calculation looks to decide the position of the right-most skeleton. We want to right-aligned the skeleton. https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/components/Skeletons/SearchRowSkeleton.tsx#L178-L180

The problem here is that when there is a scrollbar, the width of the search central pane is reduced, but since we used the windowWidth - lhn width to get the central pane width, the value is the same because windowWidth isn't reduced when there is a scrollbar. So, the skeleton position doesn't change either, thus getting cut off.

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

To solve it, first, we can get the width of the skeleton container from onLayout https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/components/Skeletons/SearchRowSkeleton.tsx#L124

const [containerWidth, setContainerWidth] = useState(0);

<View style={[styles.flex1, containerStyle]} onLayout={(e) => setContainerWidth(e.nativeEvent.layout.width)}>

Then, use the containerWidth on the calculation like this:

x={containerWidth - rightSideElementWidth - gapWidth - centralPanePadding}

We need to do the same for this skeleton https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/components/Skeletons/SearchRowSkeleton.tsx#L170-L172

x={containerWidth - rightButtonWidth - gapWidth - centralPanePadding - gapWidth - rightSideElementWidth}

What alternative solutions did you explore? (Optional)

Since we don't show scroll indicator for scroll page, https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/components/Search/index.tsx#L491

we can just hide it for the skeleton too by passing the showsVerticalScrollIndicator to EmptyStateComponent.

https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/pages/Search/EmptySearchView.tsx#L200-L211

and then use it here. https://github.com/Expensify/App/blob/64eaf2fdd357bd06008adfb9c0c049e09cd375b9/src/components/EmptyStateComponent/index.tsx#L85-L88

bernhardoj avatar Nov 28 '24 16:11 bernhardoj

Hi @deetergp and @Pujan92 - this is related to https://github.com/Expensify/App/pull/51913, so assigning you to review.

jliexpensify avatar Nov 28 '24 22:11 jliexpensify

@deetergp, @Pujan92, @jliexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] avatar Dec 02 '24 09:12 melvin-bot[bot]

Waiting on @deetergp and @Pujan92 to weigh in

jliexpensify avatar Dec 02 '24 23:12 jliexpensify

@bernhardoj's proposal sounds reasonable to me. What do you think @Pujan92?

deetergp avatar Dec 04 '24 07:12 deetergp

Yes @deetergp but I am not able to reproduce it in my system.

https://github.com/user-attachments/assets/259acb8a-39b2-4b0a-a7ac-d527b416f874

Pujan92 avatar Dec 05 '24 12:12 Pujan92

@Pujan92 please try to always show the scrollbar image

bernhardoj avatar Dec 05 '24 14:12 bernhardoj

I am πŸ‘ with the @bernhardoj's alternative solution of passing showsVerticalScrollIndicator prop to the ScrollView.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

Pujan92 avatar Dec 06 '24 11:12 Pujan92

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

melvin-bot[bot] avatar Dec 06 '24 11:12 melvin-bot[bot]

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

melvin-bot[bot] avatar Dec 06 '24 23:12 melvin-bot[bot]

Triggered auto assignment to @shawnborton (Design), see these Stack Overflow questions for more details.

melvin-bot[bot] avatar Dec 06 '24 23:12 melvin-bot[bot]

Current assignee @Pujan92 is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] avatar Dec 06 '24 23:12 melvin-bot[bot]

Please disregard @shawnborton β€” I accidentally clicked the wrong tag :ohnothing:

deetergp avatar Dec 06 '24 23:12 deetergp

PR is ready cc: @Pujan92

bernhardoj avatar Dec 07 '24 09:12 bernhardoj

Is this some kind of regression or was it always set up this way? cc @filip-solecki

shawnborton avatar Dec 09 '24 13:12 shawnborton

Tbh it's hard to say, maybe there were some changes that broke up the calculations or it is uncovered scenario.

filip-solecki avatar Dec 09 '24 13:12 filip-solecki

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

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.74-8 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/53730

If no regressions arise, payment will be issued on 2024-12-19. :confetti_ball:

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

  • @Pujan92 requires payment through NewDot Manual Requests
  • @bernhardoj requires payment through NewDot Manual Requests

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

@Pujan92 @jliexpensify @Pujan92 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

melvin-bot[bot] avatar Dec 12 '24 01:12 melvin-bot[bot]

Payment Summary

  • C: @bernhardoj $250 (to be paid via ND)
  • C+: @Pujan92 $250 (to be paid via ND)

jliexpensify avatar Dec 16 '24 03:12 jliexpensify

@jliexpensify this isn't a regression of the mentioned PR, and it is a new fresh issue. So I am also eligible for the payment.

Pujan92 avatar Dec 16 '24 03:12 Pujan92

Cool, thanks for letting me know - will add you in.

jliexpensify avatar Dec 16 '24 03:12 jliexpensify

Hi @deetergp - I'm headed OOO from the 18th - 29th, do you mind closing this GH when the payment date passes?

Payment Summary is here.

@Pujan92 - is there a need for a checklist?

jliexpensify avatar Dec 17 '24 04:12 jliexpensify

Hi @deetergp - I'm headed OOO from the 18th - 29th, do you mind closing this GH when the payment date passes?

Will do @jliexpensify! Enjoy our OOO time πŸŽ‰

deetergp avatar Dec 17 '24 16:12 deetergp

Thanks Scott, have a great Holiday Break!

@Pujan92 since it's my last day today, I want to confirm that there's no checklist?

jliexpensify avatar Dec 17 '24 20:12 jliexpensify

@jliexpensify I think checklist isn't needed here.

Pujan92 avatar Dec 18 '24 08:12 Pujan92

Great, thanks - in that case, Scott can simply close this on the 19th if there are no regressions.

jliexpensify avatar Dec 18 '24 09:12 jliexpensify

Payment Summary

Upwork Job

  • Reviewer: @Pujan92 owed $250 via NewDot
  • Reviewer: @bernhardoj owed $250 via NewDot

BugZero Checklist (@jliexpensify)

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

melvin-bot[bot] avatar Dec 19 '24 09:12 melvin-bot[bot]

$250 approved for @Pujan92

garrettmknight avatar Dec 21 '24 14:12 garrettmknight