lanitochka17 commented 4 months ago

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: 1.4-79-2 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Issue found when executing PR https://github.com/Expensify/App/pull/41344

Action Performed:

Go to staging.new.expensify.com
Create a new workspace
Delete all the categories
Go to FAB > Track expense > Manual
Track an expense
Click Categorize it from actionable whisper
Select the workspace from Step 2

Expected Result:

The folder icon in empty state screen should be centered

Actual Result:

The folder icon in empty state screen is not centered. It is aligned a bit to the left

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/31a3b6f8-4a43-4a4b-81e2-2f8c72955078

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010e432e93c8345898
Upwork Job ID: 1798238220399464448
Last Price Increase: 2024-06-05
Automatic offers:

jjcoffee | Reviewer | 102614841
tienifr | Contributor | 102614843

Issue Owner

Current Issue Owner: @abekkala

melvin-bot[bot] commented 4 months ago

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

github-actions[bot] commented 4 months ago

: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:

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

lanitochka17 commented 4 months ago

@hayata-suenaga FYI 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

hayata-suenaga commented 4 months ago

missed this. working on it right now

hayata-suenaga commented 4 months ago

I don't think we have to block deploy for this. removing the deploy blocker label

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @abekkala (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.

dragnoir commented 4 months ago

Proposal

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

Web - Categorizing - Folder icon in empty state screen is not centered

What is the root cause of that problem?

SVG icon has space on the right here

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

Adjust or edit the SVG icon to remove the space on the right.

<BlockingView
    icon={Illustrations.EmptyStateExpenses}
-  iconWidth={variables.modalTopIconWidth}
+ iconWidth={184}
-  iconHeight={variables.modalTopIconHeight}
+ iconHeight={116}
  title={translate('workspace.categories.emptyCategories.title')}
  subtitle={translate('workspace.categories.emptyCategories.subtitle')}
  contentFitImage="contain"
                    />

What alternative solutions did you explore?

Using an SVG tool, we can adjust the original SCG file, not the width/height to fit the size we need 200/164

because the SVG icon has a size of 184/116 so the contentFitImage="contain" can't make an SVG bigger than the original size

Tony-MK commented 4 months ago

Proposal

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

Web - Categorizing - Folder icon in empty state screen is not centered

What is the root cause of that problem?

Unlike the workspace page, the IOURequestStepCaterogy page has a block view with the contentFitImage attribute.

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/iou/request/step/IOURequestStepCategory.tsx#L172-L176

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L321-L326

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

We should use the or remove the contentFitImage="contain"

<BlockingView
    icon={Illustrations.EmptyStateExpenses}
    title={translate('workspace.categories.emptyCategories.title')}
    subtitle={translate('workspace.categories.emptyCategories.subtitle')}
/>

ZhenjaHorbach commented 4 months ago

Proposal

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

Web - Categorizing - Folder icon in empty state screen is not centered

What is the root cause of that problem?

The main problem with issue is that we use incorrect width and height

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/iou/request/step/IOURequestStepCategory.tsx#L172-L179

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

To fix this issue we can use width and height from WorkspaceEmptyStateSection (In this component we always pass the same image Illustrations.EmptyStateExpenses )

                width={184}
                height={116}

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L321-L325

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/components/WorkspaceEmptyStateSection.tsx#L26-L30

What alternative solutions did you explore? (Optional)

NA

tienifr commented 4 months ago

Proposal

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

What is the root cause of that problem?

In IOURequestStepCategory, we render the empty category component by using BlockingView with the wrong width and height attribute

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/iou/request/step/IOURequestStepCategory.tsx#L172-L179

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

We implemented the WorkspaceEmptyStateSection component to render the empty category component. Thus, we should use WorkspaceEmptyStateSection instead of BlockingView in IOURequestStepCategory page

With this approach, we need to adjust some CSS attributes in WorkspaceEmptyStateSection to use some styles from the parent component like backgroundColor, textColor,...

What alternative solutions did you explore? (Optional)

NA

ZhenjaHorbach commented 4 months ago

Proposal

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

Web - Categorizing - Folder icon in empty state screen is not centered

What is the root cause of that problem?

SVG icon has space on the right here

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

Adjust or edit the SVG icon to remove the space on the right.
<BlockingView
    icon={Illustrations.EmptyStateExpenses}
-  iconWidth={variables.modalTopIconWidth}
+ iconWidth={184}
-  iconHeight={variables.modalTopIconHeight}
+ iconHeight={116}
  title={translate('workspace.categories.emptyCategories.title')}
  subtitle={translate('workspace.categories.emptyCategories.subtitle')}
  contentFitImage="contain"
                    />
What alternative solutions did you explore?

Using an SVG tool, we can adjust the original SCG file, not the width/height to fit the size we need 200/164

because the SVG icon has a size of 184/116 so the contentFitImage="contain" can't make an SVG bigger than the original size

@dragnoir If you update your proposition and decide to use someone else's solution Please, leave a comment

dragnoir commented 4 months ago

@ZhenjaHorbach I added the alternative solutions. Did anyone posted using an SVG tools before me?

ZhenjaHorbach commented 4 months ago

@ZhenjaHorbach I added the alternative solutions. Did anyone posted using an SVG tools before me?

I'm talking about the main solution )

dragnoir commented 4 months ago

@ZhenjaHorbach it's the same, I just added the values 184/116 and mentioned the difference.

Adjust or edit the SVG icon to remove the space on the right.

this is my main proposal, I just didn't mention the new values.

ZhenjaHorbach commented 4 months ago

@ZhenjaHorbach it's the same, I just added the values 184/116 and mentioned the difference.

Adjust or edit the SVG icon to remove the space on the right.

this is my main proposal, I just didn't mention the new values.

Seriously ?) As for me, you just gave the most abstract solution without any references and without correct values

And then you added a solution like mine after 1 hour

But I don't want to argue

Just take note @jjcoffee When you will check propositions

jjcoffee commented 4 months ago

@dragnoir @ZhenjaHorbach Let's try to keep a good tone here. I know it can be frustrating when you notice edits to other proposals, but these will generally be taken into account by the C+ reviewing. If you feel the C+ has missed something in their review, you're welcome to point it out once the review is complete. Too much back and forth before the review leads to a messy issue that's harder for everyone to read through. Thanks! :pray:

jjcoffee commented 4 months ago

I think @tienifr has provided the tidiest solution here. This bug appeared because of essentially reimplementing WorkspaceEmptyStateSection with the wrong width attribute, so it makes sense to use it here to prevent future bugs. Happy to go with them!

@dragnoir The SVG doesn't have the wrong width as you claim in your RCA. @Tony-MK I'm not sure if your solution would help given the wrong width attribute is used. @ZhenjaHorbach As above, it makes more sense to just use WorkspaceEmptyStateSection as it reduces duplicate code and avoids the potential for future regressions.

:ribbon::eyes::ribbon: C+ reviewed

melvin-bot[bot] commented 4 months ago

Current assignee @hayata-suenaga is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

dragnoir commented 4 months ago

@jjcoffee what do you mean?

The SVG doesn't have the wrong width as you claim in your RCA.

is iconWidth={variables.modalTopIconWidth wish is equal to 200px is the correct width?

dragnoir commented 4 months ago

@jjcoffee we can't use WorkspaceEmptyStateSection because this component has a background by default styles.cardSectionContainer

https://github.com/Expensify/App/blob/66b4072db8ec86a8eed2d6e765a52eae3c9c68d4/src/components/WorkspaceEmptyStateSection.tsx#L25C43-L25C70

If we go with WorkspaceEmptyStateSection we will need to add props to change the background, spacing, ...and I don't think this is worthy it here. The component is created for other specific needs.

Can you pls check again?

ZhenjaHorbach commented 4 months ago

I think @tienifr has provided the tidiest solution here. This bug appeared because of essentially reimplementing WorkspaceEmptyStateSection with the wrong width attribute, so it makes sense to use it here to prevent future bugs. Happy to go with them!

@dragnoir The SVG doesn't have the wrong width as you claim in your RCA. @Tony-MK I'm not sure if your solution would help given the wrong width attribute is used. @ZhenjaHorbach As above, it makes more sense to just use WorkspaceEmptyStateSection as it reduces duplicate code and avoids the potential for future regressions.

🎀👀🎀 C+ reviewed

Actually I made this changes inside IOURequestStepCategory And I think the current sizes are a little incorrect So we have to fix sizes and use the same sizes as WorkspaceCategoriesPage

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx#L321-L325

But the reason I didn't use WorkspaceEmptyStateSection is because to make this component valid We need to pass exactly the same styles as for BlockingView for WorkspaceEmptyStateSection

https://github.com/Expensify/App/blob/4e4715feb25252b4c47b812807fdd1f071145825/src/components/BlockingViews/BlockingView.tsx#L129

And this logic seemed strange to me What we're trying to do is make a specific component(WorkspaceEmptyStateSection) look like a generic one(BlockingView)

But I don't mind the decision to pass styles for WorkspaceEmptyStateSection 😅

Tony-MK commented 4 months ago

Sorry for the quick proposal, but I wanted to mention that we should reuse the WorkspaceEmptyStateSection.

@tienifr got it 👍 , I think it's the right solution.

hayata-suenaga commented 4 months ago

Thank you very much everyone for your proposals. We'll go with @tienifr's proposal this time 😄

melvin-bot[bot] commented 4 months ago

📣 @jjcoffee 🎉 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] commented 4 months ago

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

dragnoir commented 4 months ago

@jjcoffee can you please check https://github.com/Expensify/App/issues/43064#issuecomment-2150117919 !

hayata-suenaga commented 4 months ago

@jjcoffee can you check the comment above?

jjcoffee commented 4 months ago

@dragnoir The need to adjust styles in that component is mentioned in the proposal that I have selected. The whole point of having a reusable component is so that we can reuse it across the app. Needing to tweak the style is a necessary part of it being reusable across different screens. The benefit of not having to re-declare the icon sizes (what if they change?) far outweighs having to declare the style.

dragnoir commented 4 months ago

@jjcoffee pls check again

https://github.com/Expensify/App/blob/90dee7accae79c49debf30354c160cab6c52c423/src/components/WorkspaceEmptyStateSection.tsx#L28-L29

The WorkspaceEmptyStateSection uses a hard coded width and height. Only icons with those sizes will fit, all the other icons will not be centered. Small icons will be on the left and bigger icons will be cutted.

So for this new proposal we will need to:

add a new prop containerStyle to adjust the position to be centered
add a new prop for if we need to use containerStyle
add a new prop shouldStyleAsCard for if it should use a background or not
we may also need to add props for width and height.

Do you think this is better that just assigning the right width and hight for BlockingView? We already use BlockingView in many UIs and we assign the width/height.

The problem is the main SVG icon is small, if the design design can update to bigger one, we don't need to change any code!!

Anyway, thanks again for your review. I just think it's not worthy all those changes.

jjcoffee commented 4 months ago

@dragnoir WorkspaceEmptyStateSection is only used with the Illustrations.EmptyStateExpenses icon, so it will always have that specific size that is hardcoded in the component. I'm not sure the reason why the icon isn't also hardcoded in the component given the component's name, but this isn't really a generic component for inserting any old icon.

Again it's not much of a trade-off to have to add a few more props to allow the component and thus the empty state icon to be generic enough to be used in other parts of the app. The benefit is clear - if we ever change the icon we only have one place to update the sizing.

jjcoffee commented 3 months ago

Nearly there with the PR, just waiting for an additional screenshot and we should be good to go!

jjcoffee commented 3 months ago

PR approved from my side.

hayata-suenaga commented 3 months ago

I've been OOO. I approved and merged the PR!

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.84-3 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/43161

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

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

@jjcoffee requires payment automatic offer (Reviewer)
@tienifr requires payment automatic offer (Contributor)

melvin-bot[bot] commented 3 months ago

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:

[ ] [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@jjcoffee] 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:
[ ] [@jjcoffee] 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:
[ ] [@jjcoffee] Determine if we should create a regression test for this bug.
[ ] [@jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

melvin-bot[bot] commented 3 months ago

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

If no regressions arise, payment will be issued on 2024-06-28. :confetti_ball:

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

@jjcoffee requires payment automatic offer (Reviewer)
@tienifr requires payment automatic offer (Contributor)

melvin-bot[bot] commented 3 months ago

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:

[ ] [@jjcoffee] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@jjcoffee] 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:
[ ] [@jjcoffee] 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:
[ ] [@jjcoffee] Determine if we should create a regression test for this bug.
[ ] [@jjcoffee] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[ ] [@abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

abekkala commented 3 months ago

PAYMENT SUMMARY FOR JUN 28

Fix: @tienifr [$250, if no regressions] Offer Link
PR Review: @jjcoffee [$250, if no regressions] Offer Link please complete checklist

jjcoffee commented 3 months ago

The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/41344
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/41344/files#r1654468598
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: N/A
Determine if we should create a regression test for this bug. Yes
If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

Create a new workspace
Delete all the categories
Go to FAB > Track expense > Manual
Track an expense
Click Categorize it from the actionable whisper
Select the workspace from Step 2
Verify that the folder icon is centered

Do we agree 👍 or 👎

abekkala commented 3 months ago

@tienifr @jjcoffee - payments sent and contracts ended - thank you! 🎉

Expensify / App

[HOLD for payment 2024-06-28] [$250] Web - Categorizing - Folder icon in empty state screen is not centered #43064

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore?

Proposal

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

What is the root cause of that problem?

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

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore?

PAYMENT SUMMARY FOR JUN 28