-
Notifications
You must be signed in to change notification settings - Fork 987
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
fix(identifier): emoji hash line height #17386
Conversation
Jenkins BuildsClick to see older builds (12)
|
@@ -66,4 +66,5 @@ | |||
{:margin-top 2}))) | |||
|
|||
(def emoji-hash | |||
{:margin-top 12}) | |||
{:margin-top 12 | |||
:line-height 20.5}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure about .5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the lineHeight of 20.5
matches the designs' spacing and height.
The design uses regular images in a row for the emoji hash while we use text for this, and the lineHeight of the text used initially was 21.75
(paragraph-1
in typography).
74% of end-end tests have passed
Failed tests (11)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (32)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
Best to send it to design review, as it is follow-up on design issue. e2e failures rare not related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the same but this was the result on Figma |
dbc2073
to
c881d3a
Compare
@Francesca-G Can you please retry 🙂. I just confirmed again and it matches the height. |
it doesn't match on my side 🤔 I don't know if it's a screen res problem, I'm using an iPhone 13 to test this. As you can see all the alignments inside the card don't match the design as well, so I think that's affecting the card height (which should be 184px) |
Hi @Francesca-G |
yes you need to be using iphone 11 Pro to verify with Figma in this way, otherwise the sizes won't work. Additionally the screenshot needs to be from the phone/simulator (e.g not the laptop screenshot) so that the size is correct. |
Just realised we have this documented here : https://github.com/status-im/status-mobile/blob/develop/doc/pixel-perfection.md |
c881d3a
to
ac7002b
Compare
Hi @Francesca-G 🙂. Can this be merged? |
Yes, apologies again for the device resolution problem we encountered. |
ac7002b
to
1c105f5
Compare
Fixes #17330, follow up to #17293
Summary
The identifier card height in the onboarding flow should match the design
Platforms
status: ready