[16845] Wrong buttons background color on community home screen's page nav #17003
Conversation
Jenkins BuildsClick to see older builds (78)
|
There was a problem hiding this comment.
@ibrkhalil did you check with design about this is how it should work? 🤔
Also I noticed in the preview images you added the community being used has no banner image, it's better to avoid testing with this community as banner images are mandatory to make and so it's better to test with a community that has one in place (i.e not generated in mobile)
Let me test it on a community with a banner image |
yep because in the designs if the configuration of buttons does not change based on bottom sheet opening then the issue is not related to this and it's probably just how the buttons are swapping based on scroll height of the page. |
86% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (37)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Hi @ibrkhalil thank you for PR. Here is a found issue: ISSUE 1: Background is slightly visible for buttons when overlay layer is appliedSteps:
Actual result:Background for community buttons is slightly visible
Expected result:The background behind the community buttons should have a lighter appearance, in accordance with the design. Device:
|
|
Blocked by: #17017 |
|
@VolodLytvynenko Might be fixed in #17031 |
PR is merged, Kindly take a look @VolodLytvynenko so that we can close this issue. |
Thank you. I'll keep you updated if this issue persists. Otherwise, I will close this PR |
|
hi @ibrkhalil the issue 1 is resolved. ISSUE 2: [Android] Buttons are not blurred when overlay layer is appliedSteps
Actual result:The buttons in the community are not blurred when the overlay layer is applied. Expected result:The buttons should be blurred in accordance with the design when the overlay layer is applied. Device:
|
|
@ibrkhalil Could you please check this new issue if it can be addressed within the scope of this PR? Let me know if it would be better to address it separately. I can create a separate follow-up for it if needed. Thank you ISSUE 3: The [x] button is not blurred regardless the overlay layer is applied or notSteps
Actual result:The [x] button is not blurred regardless of whether the overlay layer is applied or not.
Expected result:The [x] button should be blurred in accordance with the design when the overlay layer is applied or not. |
|
I rewrote the solution, Re-requesting reviews |
| @@ -20,7 +20,7 @@ | |||
| :blur :grey}) | |||
|
|
|||
| (defn- page-nav-base | |||
| [{:keys [margin-top background on-press accessibility-label icon-name] | |||
| [{:keys [margin-top background on-press accessibility-label icon-name overlay-shown?] | |||
There was a problem hiding this comment.
seems like a good solution, perhaps it needs a commit to explain why it is needed? 🤔
There was a problem hiding this comment.
You mean in the commit message or a comment?
There was a problem hiding this comment.
Just in case it's a comment, I'll do it now
There was a problem hiding this comment.
also a nit: perhaps behind-overlay? is a clearer name for this prop? 🤨
| @@ -48,7 +50,9 @@ | |||
| :icon-only? icon-name | |||
| :size 32 | |||
| :accessible true | |||
| :background (when (button-properties/backgrounds background) background)) | |||
| :background (if overlay-shown? | |||
There was a problem hiding this comment.
perhaps put this logic into a simple helper function and leave a comment by that function explaining it's purpose there?
There was a problem hiding this comment.
We just check if the background is within a set of button types, Here's the definition of backgrounds.
There was a problem hiding this comment.
I more so meant this because of the overlay part. It's somewhat of a design hack and so isolating to its own function and leaving the comment there hopefully makes the reason for this more apparent/easy to see
There was a problem hiding this comment.
I think it's better to have a comment explaining it than having a function for it.
There was a problem hiding this comment.
Code wise, I don't have much to add. Thanks @ibrkhalil, I think now it's a matter of getting the design approved and well tested for regressions.
|
@VolodLytvynenko Can you please take a look? |
58% of end-end tests have passedFailed tests (18)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (25)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
@ibrkhalil Thank you for PR. No issues from my side. PR is ready to be merged |
fixes #16845
Summary
The state of whether to show the blurred version of the buttons didn't cater for bottom sheet being open (Specifically when it's overlay is visible) And blurring on Android showed in a wrong way because of a limitation/incosistency of blurring between iOS and Android
This is not 100% matching figma, But it's as close as possible.
status: ready