Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-10-07][$250] Loading skeleton for Settings header is slightly off #49548

Closed
1 of 6 tasks
m-natarajan opened this issue Sep 20, 2024 · 19 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Sep 20, 2024

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.39-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @shawnborton
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726813572232319

Action Performed:

  1. Go to staging.new.expensify.com
  2. Click Account settings > Trouble shooting
  3. Click Clear cache and restart > Reset and Refresh
  4. Observe the Avatar and Username in LHN

Expected Result:

No visual issues

Actual Result:

When the account is loading on Settings, the location of the loading skeleton is slightly off from where the actual avatar and username will go

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

CleanShot.2024-09-20.at.08.25.18.mp4
Recording.565.mp4

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021838488394004973335
  • Upwork Job ID: 1838488394004973335
  • Last Price Increase: 2024-09-24
  • Automatic offers:
    • c3024 | Reviewer | 104109595
Issue OwnerCurrent Issue Owner: @c3024
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 20, 2024
Copy link

melvin-bot bot commented Sep 20, 2024

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

@nkdengineer
Copy link
Contributor

nkdengineer commented Sep 20, 2024

Edited by proposal-police: This proposal was edited at 2024-09-21 04:28:05 UTC.

Proposal

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

When the account is loading on Settings, the location of the loading skeleton is slightly off from where the actual avatar and username will go

What is the root cause of that problem?

Styles change

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

We should add margin-left with negative value here

App/src/styles/index.ts

Lines 2828 to 2830 in 513e6b3

avatarSectionWrapperSkeleton: {
width: '100%',
},

What alternative solutions did you explore? (Optional)

Or we can reduce the value of startPositionX

This is results when we set marginLeft = -8px or startPositionX = 22px

Screen.Recording.2024-09-21.at.02.03.42.mov

@Krishna2323
Copy link
Contributor

Proposal


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

Loading skeleton for Settings header is slightly off

What is the root cause of that problem?

The cx value is set to 30.

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


Reduce it to 20 or something else as per requirement.

What alternative solutions did you explore? (Optional)

Result

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@bernhardoj
Copy link
Contributor

Proposal

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

The setting avatar skeleton loader is not aligned to the left.

What is the root cause of that problem?

The cx value here is set to 30.

<Circle
cx={startPositionX}

The cx value tells the position of the x center of the circle. If we set it to 0, then only the right half of the circle will be shown.
image

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

To align the circle to the left of the view, then we need to set the cx to be the half size of the circle, which is the same as its radius. So, we can put the radius value here as the startPositionX.

const startPositionX = avatarPlaceholderRadius;
image

Copy link

melvin-bot bot commented Sep 23, 2024

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

@kadiealexander kadiealexander added the External Added to denote the issue can be worked on by a contributor label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

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

@melvin-bot melvin-bot bot changed the title Loading skeleton for Settings header is slightly off [$250] Loading skeleton for Settings header is slightly off Sep 24, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Sep 24, 2024
@QasimBham
Copy link

Proposal
Please re-state the problem that we are trying to solve in this issue.
The setting's icon is not aligned to the left.
What is the root cause of that problem?
The cx value is set to 30
What changes do you think we should make in order to solve the problem?
The cx value should be set to half of it's original value so that it'll be aligned to left perfectly.
What alternative solutions did you explore? (Optional)

Copy link
Contributor

user Your proposal will be dismissed because you did not follow the proposal template.

@QasimBham
Copy link

Proposal

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

The setting's avatar is not left aligned

What is the root cause of that problem?

The cx value. is set to 30

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

The cx value should be set to half of the original value. This should solve the left alignment icon issue

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

@c3024
Copy link
Contributor

c3024 commented Sep 24, 2024

Thank you all for your proposals. All correctly identified the Avatar Skeleton Circle centre position as the reason for the mismatch.

As far as my experience and understanding w.r.t. CSS/SVG issues goes, identifying the correct attribute and its accurate value (considering other existing styles/attributes) is seen valuable enough to prioritise a proposal over others.

@bernhardoj 's proposal identifies the circle position exactly to match the loaded avatar position. I think we can go with his proposal.

🎀 👀 🎀 C+ Reviewed

Copy link

melvin-bot bot commented Sep 24, 2024

Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 24, 2024
Copy link

melvin-bot bot commented Sep 24, 2024

📣 @c3024 🎉 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 melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 25, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @c3024

@kadiealexander kadiealexander changed the title [$250] Loading skeleton for Settings header is slightly off [HOLD for payment 2024-10-08][$250] Loading skeleton for Settings header is slightly off Oct 8, 2024
@kadiealexander kadiealexander changed the title [HOLD for payment 2024-10-08][$250] Loading skeleton for Settings header is slightly off [HOLD for payment 2024-10-07][$250] Loading skeleton for Settings header is slightly off Oct 8, 2024
@kadiealexander
Copy link
Contributor

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:

  • [@c3024] The PR that introduced the bug has been identified. Link to the PR:
  • [@c3024] 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:
  • [@c3024] 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:
  • [@c3024] Determine if we should create a regression test for this bug.
  • [@c3024] 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.
  • [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@kadiealexander
Copy link
Contributor

kadiealexander commented Oct 8, 2024

Payouts due:

Upwork job is here.

@bernhardoj
Copy link
Contributor

Requested in ND.

@c3024
Copy link
Contributor

c3024 commented Oct 8, 2024

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:

  • [@c3024] The PR that introduced the bug has been identified. Link to the PR: Delegate access: Add account switcher #47338
  • [@c3024] 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/47338/files#r1791590530
  • [@c3024] 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 discussion was started because this could not have been identified earlier.
  • [@c3024] Determine if we should create a regression test for this bug. Yes
  • [@c3024] 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

  1. Go to Settings > Troubleshoot
  2. Press Clear cache and restart > Reset and Refresh
  3. Go back to Settings if it is a narrow layout
  4. Verify that the position of the circle on top left remains the same as the skeleton view's even after the avatar is loaded

@JmillsExpensify
Copy link

$250 approved for @bernhardoj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants