-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-02-19] [Wave8] [Ideal Nav] Page header design follow up #35571
Comments
Triggered auto assignment to @stephanieelliott ( |
Hey, thanks for creating this issue. I will work on this! |
@kosmydel Thank you! Are you able to address this one today? |
Sure. I can switch my priorities. |
I believe it should be 72px as every size is a multiple of 4. |
@Expensify/design Can you please confirm the value here? |
I have a feeling it was 73px to account for a potential 1px border that happens. That being said, happy to use 72px since we rarely have the border here (and typically it's only on a chat header). That being said, can we confirm what this value was before the ideal nav PR? I think we should just revert it to what it was previously. |
I am seeing 65px on staging. |
Can you share a screenshot of that please? |
Interesting... could have sworn that would have been 72/73. |
Yeah I was thinking that the "correct" height was 72px but we made it 73px in Figma to account for the border. But since we're not really using borders on headers anymore (until scroll—is that still a thing?) I think we should just pick a number and standardize. My vote would be for either Once we decide, I think the @Expensify/design team should make sure this component is updated in Figma (it could probably use a little love anyways since a lot has changed with these headers since the component was first created). |
I think we should do 72px personally, it does feel like it gives it a teeny more breathing room. |
That sounds perfectly great to me. Can it be decided? |
Decided! 72 it is. |
@kosmydel Please update the height as stated above. |
@kosmydel do you think you can handle this issue together? |
I'm OOO, I will fix it tomorrow.
Sure, I can have a look at this tomorrow. |
Looks like the PR was updated and is on staging now! |
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results. If a regression has occurred and you are the assigned CM follow the instructions here. If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.39-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-02-19. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
$500 to @shubham1206agra for testing and review No regression test required for this I think |
Payment Summary
BugZero Checklist (@stephanieelliott)
|
@shubham1206agra I extended the offer to you in Upwork, please accept when you get a chance: https://www.upwork.com/jobs/~018b9bc79836e2ae70 |
@stephanieelliott Accepted |
Thanks @shubham1206agra, all paid! |
Coming from here
Here's the plan:
Issue Owner
Current Issue Owner: @stephanieelliottThe text was updated successfully, but these errors were encountered: