Make color selector take full device width to match Figma #17465
Conversation
Jenkins BuildsClick to see older builds (84)
|
There was a problem hiding this comment.
Hi @ibrkhalil,
Thank you for the PR.
We need to make the UI look the same as Figma by making the margin between circles flexible as per screen size.
Please checkout this discord conversation for more information.
|
@ibrkhalil I didn't check the issue description. It seems the issue is not addressed? The circles need to have dynamic margins based on screen width so that part of the last circle is visible |
The spacing between them should allow it to show if we didn't add the horizontal margin. |
Are you sure it will show on all devices? Removing the margins may make the last circle show on some devices and not on others I think |
You're right, But what really makes me want to make padding go away is that in the designs circles go edge to edge. |
53% of end-end tests have passedFailed tests (20)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (23)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hi @ibrkhalil! It seems that current implementation does not fix the problem for all devices. Can we have some universal solution? ISSUE 1 The whole circle is visible instead of part of the next one (Samsung Galaxy A52)Such implementation does not fix the problem as it is still not obvious that colour pickers are scrollable Here is a screenshot form Samsung Galaxy A52
|
ISSUE 2 Colour pickers stick to the left edge instead of to be aligned with other elements on the pageColour pickers selector starts from the very edge while should be alligned with other elements and have more space from the left side. Actual result:
Expected result: 
|
|
Hey @ibrkhalil any update on this? |
|
Not yet, Working on Top bar animation bugs atm. |
|
I was able to come up with a ratio to calculate a dynamic margin that'll match Figma for iPhone 11 Pro and adjust the value accordingly for other screens.
|
|
@pavloburykh Ready again |
| :border-width 4 | ||
| :border-radius 24 | ||
| :margin-right (-> window-width | ||
| (- constants/IPHONE_11_PRO_VIEWPORT_WIDTH) |
There was a problem hiding this comment.
I think this deserves a comment - or put this in a private function to help explain the idea/logic here 👍
84% of end-end tests have passedFailed tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (38)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@ibrkhalil thanx for the fix! LGTM. PR is ready for design review cc @Francesca-G |
|
@pavloburykh Do you think this should be added to the wallet creation screen? |
@ibrkhalil According to Figma wallet color pickers are the same. 
Currently it looks like this: Android 12, Samsung Galaxy A52
IOS 16.1, iPhoneX
@ibrkhalil please reach someone from wallet team on this question. I believe @J-Son89 might be the right person. Meanwhile, I propose not to block this PR from merging. Just create a followup after synchronising with wallet team. |
| :default-selected :blue | ||
| :on-change on-change | ||
| :window-width window-width | ||
| :container-style {:padding-left (int (/ window-width 18.75))}}]]] |
There was a problem hiding this comment.
where is this magic 18.75 coming from?
There was a problem hiding this comment.
I wanted to add padding 20 like the design, But I wanted to make it responsive.
18.75 = iPhone 11 Pro VW / 20
There was a problem hiding this comment.
Ok, well this should be in a def and then leave a comment explaining the value or some means to communicate this idea 👍
|
@ibrkhalil, @pavloburykh this color picker is used on several screens in the wallet. If you search for quo/color-picker you will find the uses Ibrahem. Create account page, edit account page are some examples. |
I know where it's used Jamie, But my point is should I fix this bug there too or not? |
|
Ah got it, we will be making adjustments to the wallet pages anyway so feel free to leave it to keep this pr scope smaller. 👍 |
|
Hi @ibrkhalil, I am getting the following bug in the color picker in Wallet
|
Thanks for reporting it Omar, I created a PR to fix it here |
fixes #17443
Fixes a UI issue on smaller screen and older devices where the ScrollView that renders the list of colors didn't show that it's scrollable because it had horizontal padding alongside the spacing which led #17443
Also on Figma, The width is set to take the whole available screen width, So The padding was unnecessary.
status: ready