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 2023-01-30] Profile picture on profile page should have 0 px padding or margin from top navigation #14182

Closed
6 tasks
kavimuru opened this issue Jan 10, 2023 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Jan 10, 2023

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


Action Performed:

  1. Open the app
  2. Open settings and check the padding from top navigation for profile image
  3. Open Workspaces
  4. Open any Workspace and check the margin from top navigation for Workspace profile
  5. Open general settings and check the margin from top navigation for Workspace profile
  6. Open any report
  7. Click on any user name and check the padding from top navigation for user profile
  8. Open settings
  9. Open profile and check the padding or margin from top navigation for profile image

Expected Result:

All instances where the Profile image appears in the app should have a consistent amount of top padding.

Actual Result:

Profile image has no margin or padding on the top.
Note: (Other pages with profile maintains 20 px margin or padding from navigation)

Workaround:

unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: 1.2.51-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
Notes/Photos/Videos:
image (2)
image (3)
image
3
2
1

Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673281072815909

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b073cc0e3a5005d6
  • Upwork Job ID: 1613974508062380032
  • Last Price Increase: 2023-01-13
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 10, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Jan 10, 2023
@joekaufmanexpensify
Copy link
Contributor

Discussing here.

@arosiclair arosiclair self-assigned this Jan 11, 2023
@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Jan 11, 2023

Working on triaging this:

From this SO: https://stackoverflow.com/c/expensify/questions/14418

  • The "bug" is actually a bug: Y
  • The bug is not a duplicate report of an existing GH (close the GH and add any novel details to the original GH instead): N/A
  • The bug is reproducible, following the reproduction steps: Y
  • If you're unable to reproduce the bug, add the Needs reproduction label. Comment on the issue outlining the steps you took to try to reproduce the bug, your results and tag the issue reporter and the Applause QA member who created the issue. Ask them to clarify reproduction steps and/or to check the reproduction steps again. Close issue.: N/A
  • The GH template is filled out as fully as possible -- this means the GH body and title are clear (ie. could an external contributor understand it and work on it?): Y
  • The GH was created by an Expensify employee or a QA tester: Y
  • If the bug is an OldDot issue, you should decide whether it is widespread enough to justify fixing or it is better to wait for reunification. If it's better to wait, close the GH & provide this justification: N/A
  • If there's a link to Slack, check the discussion to see if we decided not to fix it: N/A
  • Decide if the GH should be resolved by an External contributor or Internal engineer, add the appropriate label: Going to keep this internal.

@joekaufmanexpensify
Copy link
Contributor

@arosiclair once this is triaged, I'm thinking you want to take this one and keep it internal?

@arosiclair
Copy link
Contributor

@arosiclair once this is triaged, I'm thinking you want to take this one and keep it internal?

Yup that's correct!

@joekaufmanexpensify
Copy link
Contributor

Sounds good!

@joekaufmanexpensify
Copy link
Contributor

Reproduced on web:

image

image

@joekaufmanexpensify
Copy link
Contributor

Okay, this is reproduced. Before we proceed, discussing what the actual fix looks like here in the slack thread. Let's hold off on proceeding until we're crisp on that.

@joekaufmanexpensify
Copy link
Contributor

Discussed in the slack thread, and the solution we want for fixing this bug is leaving no top padding on the profile page, and removing the top padding for the profile image on the Settings page, the workspace page, the workspace general settings page, and the details page accessed when clicking another user's profile image.

Updating the OP of the issue to make this clear, and then making this internal.

@joekaufmanexpensify joekaufmanexpensify added the Internal Requires API changes or must be handled by Expensify staff label Jan 13, 2023
@melvin-bot melvin-bot bot unlocked this conversation Jan 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Jan 13, 2023

Triggered auto assignment to Contributor Plus for review of internal employee PR - @eVoloshchak (Internal)

@joekaufmanexpensify
Copy link
Contributor

All yours @arosiclair !

@melvin-bot melvin-bot bot added the Overdue label Jan 16, 2023
@joekaufmanexpensify
Copy link
Contributor

Not overdue.

@joekaufmanexpensify
Copy link
Contributor

Hm, not sure why the label is still there?

@joekaufmanexpensify
Copy link
Contributor

Next step here is to begin the PR

@arosiclair
Copy link
Contributor

Working on this today

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Jan 18, 2023
@joekaufmanexpensify
Copy link
Contributor

Okay, no C+ review done here, so no payment to them is needed. We implemented this internally, so no C payment as well. So the only payment we need to make here is the $250 reporting bonus to @dhanashree-sawant

@joekaufmanexpensify
Copy link
Contributor

@eVoloshchak un-assigning as you didn't review the PR here.

@joekaufmanexpensify
Copy link
Contributor

@dhanashree-sawant, contract sent for $250! We'll issue payment after the 7 day regression waiting period, on 2023-01-30.

@joekaufmanexpensify
Copy link
Contributor

joekaufmanexpensify commented Jan 26, 2023

Besides that, only other step here is the BZ checklist. Not sure why it didn't get automatically created, so will add it manually!

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:

  • [@arosiclair ] The PR that introduced the bug has been identified. Link to the PR:
  • [@arosiclair] 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:
  • [@arosiclair ] 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: LINK
  • [@joekaufmanexpensify] 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: Added to new design testing guidelines.

@joekaufmanexpensify joekaufmanexpensify changed the title Profile picture on profile page should have 20 px padding or margin from top navigation [hold for payment 2023-01-30] Profile picture on profile page should have 20 px padding or margin from top navigation Jan 26, 2023
@joekaufmanexpensify joekaufmanexpensify added Weekly KSv2 and removed Daily KSv2 labels Jan 26, 2023
@arosiclair
Copy link
Contributor

Finding it very hard to find when exactly the spacing on top of the profile page was removed. It definitely had some spacing originally, but the profile page history is long with a lot of layout changes. Since this is a low impact bug and we had a big redesign, I'll skip that part and just post a thread on how to possibly keep this from occurring again.

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 labels Jan 30, 2023
@joekaufmanexpensify
Copy link
Contributor

Switching to daily as payment is due today.

@joekaufmanexpensify
Copy link
Contributor

@dhanashree-sawant $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Closed upwork job.

@joekaufmanexpensify
Copy link
Contributor

Proposed new regression test here.

@joekaufmanexpensify joekaufmanexpensify changed the title [hold for payment 2023-01-30] Profile picture on profile page should have 20 px padding or margin from top navigation [hold for payment 2023-01-30] Profile picture on profile page should have 0 px padding or margin from top navigation Jan 31, 2023
@joekaufmanexpensify
Copy link
Contributor

Okay, ended up not creating a new regression test here. Since this is more design focused, I instead added it to the new design testing guidelines we're creating for applause.

@joekaufmanexpensify
Copy link
Contributor

Closing as this is all set!

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. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

4 participants