Closed lanitochka17 closed 6 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:
Triggered auto assignment to @amyevans (Engineering
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
We think that this bug might be related to #vip-vsp CC @quinthar
@amyevans 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
This is not blocker. Production doesn't have env badge. Though I am curious why this was reported so late as happenning on dev/staging for a while.
Aka. prodution breadcrumbs are also misaligned
Chats - Expensify / Chats is not aligned on the same line.
Offending PR:32221
This style is causing the issue.
Remove style={styles.breadcrumbLogo}
here:
Add styles.alignItemsCenter
here:
I think this might have started after the update to breadcrumbs scaling https://github.com/Expensify/App/pull/36655 cc @dukenv0307
@Expensify/design
Hmm that is definitely possible, and if so, let's treat it like a regression and have @dukenv0307 follow up with a fix.
@mountiny I just tested locally by reverting my PR and the result is the same. Correct me if I missed st.
I think we can make it external then for $250
Job added to Upwork: https://www.upwork.com/jobs/~01c8acccc3eea77881
Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External
)
Upwork job price has been updated to $250
for whatever proposal @MahmudjonToraqulov Could you please identify what PR caused this regression, I agree this cannot be more than couple releases old
@mountiny @shawnborton @cubuspl42 Is this what you need ? Please let me know if that is the case then I will put down a proposal and also offending PR asap
UPDATED with offending PR
That looks pretty good, just make sure it covers the case where the logo doesn't have the environment label as well.
That looks pretty good, just make sure it covers the case where the logo doesn't have the environment label as well.
Hey @shawnborton. Friendly I already did it above before @godofoutcasts94. And it works well even without environment label as well.
Sounds good - I'm just here to provide some design guidance, will let the managing assignees handle the proposal process :)
managing assignees handle the proposal process
Ah sorry. I didn't check the assigners for the issue yet :).
https://github.com/Expensify/App/pull/32221 should not be offending PR. It was the first implementation. As you see screenshot, it was styled correctly.
Expensify / Chats text is not aligned
setting the height to height: variables.lineHeightSizeh1
is causing the issue here
https://github.com/Expensify/App/blob/b7a230ea75dbc4294f061c7627a24679e346641e/src/styles/index.ts#L1539-L1542
height: variables.lineHeightSizeh1
removed from breadcrumbLogo
styles fixes the issue.
Note: we cannot remove top
since there is a comment which says top is added for pixel perfect alignment
N/A
Chats - Expensify / Chats is not aligned on the same line
This is happing also because of this line - https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L43 And also related to stylings here - https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L68 and here - https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L69-L74 as well.
We need to remove the style={styles.breadcrumbLogo}
from here -
https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L43
and also we need to add mb1
here -
https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L68
and here
https://github.com/Expensify/App/blob/0a512c9f3cf1b207668ab93f3d79866c80345f88/src/components/Breadcrumbs.tsx#L71
as well
N/A
https://github.com/Expensify/App/assets/158435970/4e65fdc8-ad5a-4677-a229-f1f2b9717599
Updated Proposal and also the case will be handled were logo doesn't have environment variable as well
Triggered auto assignment to @puneetlath (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Chats - Expensify / Chats is not aligned on the same line
Come from offending PR. currently the logo is 28px and the font size is 24px, so the logo is bigger than the font size, and the alignment is not correct.
The related PR to fix alignment when changing the font size on mobile devices has been fixed in many places so to avoid another regression from the original issue we need to check the condition Browser.getBrowser()
or getPlatform()
if it's a native device applying the style change from the offending PR otherwise keep the old code
POC | Before | After |
---|---|---|
We can update header size to variables.lineHeightSizeh2
due to breadcrumsContainer set height base on 24px and lineHeightSizeh2 also calculating value base on min 24, then update headerEnvBadge position from bottom: -8
to bottom: -12
due to diff value from lineHeightSizeh2 to lineHeightSizeh1 is 4px
POC
I have updated the alternative proposal.
@suneox
Come from offending PR.
https://github.com/Expensify/App/issues/38298#issuecomment-1997397589
So how is it? 🙂
@MahmudjonToraqulov
And it works well even without environment label as well.
Would you include the relevant screenshots in the proposal, in such a case?
@aneequeahmad @godofoutcasts94
Thanks for your proposals. Unless they cover any case @MahmudjonToraqulov missed, I don't think that they are superior in any way, so I would go with the first on posted in this case.
@aneequeahmad @godofoutcasts94
Thanks for your proposals. Unless they cover any case @MahmudjonToraqulov missed, I don't think that they are superior in any way, so I would go with the first on posted in this case.
Of course @cubuspl42. Updated my PROPOSAL with new screenshot img.
@cubuspl42 If we Remove style={styles.breadcrumbLogo}
as said here it isn't aligned on the same line. if you look closely you can se 1-2 pixel difference in alignment(1.66px to be exact). There is also a comment for this style https://github.com/Expensify/App/blob/b7a230ea75dbc4294f061c7627a24679e346641e/src/styles/index.ts#L1539-L1540
Screenshot of the case when whole style is removed is here(not aligned perfectly).
That why i said we need to only have style top: 1.66
and no need to have fixed height height: variables.lineHeightSizeh1,
and its generally not a good practice to give height to the boxes containing elements(margin and padding attributes given right values should set the perfect height).
Screenshot of my proposal is here
Hey @aneequeahmad. As the main solution to the issue was removing style={styles.breadcrumbLogo}
in my first solution I haven't added positioning yet.
Let me ask friendly have u checked my second solution?
When it comes to screenshots, have you checked my screenshots?
Thanks.
Come from offending PR.
So how is it? 🙂
@cubuspl42 after revert this issue not happens
https://github.com/Expensify/App/assets/11959869/5e579100-e0ef-47f0-8abe-a5d3e04d168a
@MahmudjonToraqulov if you haven't added the positioning yet then how can alignment issue be fixed by your solution ?
Also i have checked your second solution it doesn't fix the alignment as well please look at the screenshot below closely from your 2nd solution Expensify / Chats
is not aligned(not pixel perfect).
@suneox Your video shows the alignment isn't correct. Please see the screenshot below
@suneox Your video shows the alignment isn't correct. Please see the screenshot below
@aneequeahmad your screenshot also have a little space bellow text
Are you sure your solution work well on mobile device when change font size?
In the video we can see offending PR and this is a root cause
@aneequeahmad your screenshot also have a little space bellow text
@suneox The screenshot you are referring to is the screenshot from your video please look closely.
The screenshot from my proposal is here
@suneox The screenshot you are referring to is the screenshot from your video please look closely.
@aneequeahmad Sure the screenshot you capture from my video demonstrate the rootcause but your capture from my video doesn't capture to align bottom of text
@suneox
we need to check the condition Browser.isMobile() before applying the style change from PR
I checked the diff from #36655 and I can't see any mobile-dependent code. You'd need to elaborate why your solution is mobile/desktop Web specific.
Also, this issue is said to be occurring on all platforms.
Which of our officially supported platforms is this issue occurring on? (all)
...and you mention mentions Web-specific mobile check. This is all unclear.
@aneequeahmad
As it's been suggested that there might be some mobile-specific aspect of this issue:
Have you tested your changes both on all platforms, at least mobile/desktop Web and mobile Native?
@suneox
we need to check the condition Browser.isMobile() before applying the style change from PR
I checked the diff from #36655 and I can't see any mobile-dependent code. You'd need to elaborate why your solution is mobile/desktop Web specific.
@cubuspl42
I have a mistake for condition It's should be Browser.getBrowser()
. We have to check the condition to apply the PR if the current platform is native code otherwise, keep the old code. Due to at this line using PixelRatio.getFontScale
and this line use func getValueUsingPixelRatio
also using PixelRatio.getFontScale
and this function using for native code on browser only return fixed value
Please update the proposal to describe your actual proposed solution.
Update a condition check platform before applying the offending PR
Please update the proposal to describe your actual proposed solution.
Hey @cubuspl42. You were gonna go with my solution if I'm not mistaken. LMK Have u changed your decision?
@MahmudjonToraqulov
I said "Unless they cover any case you missed".
So far, we have three proposals:
style={styles.breadcrumbLogo}
height: variables.lineHeightSizeh1
from
If the simpler approach works on all platforms, I'd prefer it. If not, other proposals might be just invalid and then we'll re-evaluate.
Ping @aneequeahmad about testing on all platforms.
Hey @cubuspl42 why is my proposal not even considered ??
@MahmudjonToraqulov suggests to just drop the
style={styles.breadcrumbLogo}
- It's simple but removes the "pixel-perfect alignment"
What about my alternative solution?
@godofoutcasts94
I keep forgetting about it. Sorry.
style={styles.breadcrumbLogo}
From my perspective, it has the same qualities as the first proposal, i.e. it looses the "pixel-perfect alignment". Also, it's last in the submitting order, so it would have to be measurably better than the one by @aneequeahmad to be selected.
But you're right that it should be included in the commentary.
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.52-0 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/37421
Action Performed:
Expected Result:
Expensify / Chats is aligned on the same line
Actual Result:
Expensify / Chats is not aligned on the same line
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit