refactor copyable-text component to accept background-color property #13764
Conversation
siddarthkay
changed the title
siddarthkay
changed the title
siddarthkay
changed the title
Jenkins BuildsClick to see older builds (15)
|
| @@ -120,5 +121,5 @@ | |||
| :underlay-color colors/black | |||
| :on-press copy-fn | |||
| :on-long-press copy-fn} | |||
| [react/view {:background-color colors/white} | |||
| [react/view {:background-color background-color} | |||
There was a problem hiding this comment.
so it will be nil if there is color in container ?
There was a problem hiding this comment.
No, I'm defaulting to white when no background-color is passed
There was a problem hiding this comment.
hi, did you tested the changes? I think you are assigning :background-color to wrong view. (Or this is the desired behavior?).
Also, please can you explain what are the use cases of passing background-color, means what you are trying to achieve?
main.mp4
There was a problem hiding this comment.
hi, did you tested the changes? I think you are assigning :background-color to wrong view. (Or this is the desired behavior?). Also, please can you explain what are the use cases of passing background-color, means what you are trying to achieve?
main.mp4
Yes my primary case for introducing this change was when there were UI screens that have dark backgrounds or transparent backgrounds, example : Share tab for switcher : https://www.figma.com/file/eDfxTa9IoaCMUy5cLTp0ys/Shell-for-Mobile?node-id=451%3A18993
If you add copyable-text component to any of the text fields on such a screen the background color is white by default which is not suitable for that use case.
I would have to pass background-color :transparent for it to work in that screen.
and that was my motivation to introduce this property in copyable-text component, I guess I should have mentioned the motivation upfront while making the PR.
There was a problem hiding this comment.
so it will be nil if there is color in container ?
I now understand the issue with using (when-not) which would produce nil if background-color property was not passed to this component, I have refactored it in latest commit to use or
The code should now work as expected.
It will accept the background-color passed and if there is no background-color passed it will use default value of colors/white
There was a problem hiding this comment.
@Parveshdhull
using copyable-text component as it exists today I get this o/p
[copyable-text/copyable-text-view {:copied-text profile-link :container-style {:background-color :transparent :width :100%}} [rn/text {:style (profile-address-content (* window-width 0.7)) :ellipsize-mode :middle :number-of-lines 1} profile-link] ]
Pay attention to the text below link to profile label
And below is the o/p after I make modification to accept background-color property :
I think this use case is particular when text color is also white..
There was a problem hiding this comment.
hi @siddarthkay,
Thank you for the explanation and video. Yes, your provided code
[copyable-text/copyable-text-view {:copied-text profile-link :container-style {:background-color :transparent :width :100%}} [rn/text {:style (profile-address-content (* window-width 0.7)) :ellipsize-mode :middle :number-of-lines 1} profile-link] ]
showed desired behavior. Looks like in the above video(by me), the list item's background color was affecting the net result.
PR looks good to me, but if changes are made for the use case of switcher's copyable text, I would recommend making these changes in that PR, so that they can be properly tested. wdyt?
There was a problem hiding this comment.
changes are made for the use case of switcher's copyable text, I would recommend making these changes in that PR, so that they can be properly tested. wdyt?
Hmmm that is also a valid explanation to keeping this change in that PR.
My initial motivation was also that since its a change to a component which can be used in other multiple PRs I moved this code to a seperate PR.
This modification also exists in the share-tab PR, I was thinking of removing it from there...
Any approach is fine by me.
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
8694a0d
to
c70e0d3
Compare
97% of end-end tests have passedFailed tests (3)Click to expandClass TestSendTxDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
Passed tests (84)Click to expandClass TestCommandsMultipleDevicesMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestOnboardingOneDeviceMerged:
Class TestRestoreOneDeviceMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestSendTxDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestPublicChatMultipleDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
|
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
c70e0d3
to
42922a8
Compare
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
fd33b6c
to
1e6ef9a
Compare
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
1e6ef9a
to
081fcd2
Compare
|
The iOS build failures are due to failing Not sure why, the permissions look okay: For now I've purged the workspace from all MacOS CI hosts. If it shows up again I can look into it further. |
98% of end-end tests have passedFailed tests (2)Click to expandClass TestPairingSyncMultipleDevicesMerged:
Class TestEnsStickersMultipleDevicesMerged:
Passed tests (85)Click to expandClass TestPairingSyncMultipleDevicesMerged:
Class TestOnboardingOneDeviceMerged:
Class TestSendTxDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestKeycardTxOneDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestRestoreOneDeviceMerged:
|
|
@siddarthkay thanks for waiting! |
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
081fcd2
to
e207997
Compare
Refactoring copyable-text component to accept background-color property. Fix case of nil Trying to sign
siddarthkay
force-pushed
the
siddarthkay-patch-copyable-text
branch
from
e207997
to
c0e2dc7
Compare
Refactoring copyable-text component to accept background-color property. Fix case of nil Trying to sign
Summary
Refactoring copyable-text component to accept background-color property.
Platforms
Areas that maybe impacted
It may or may not have an impact on all areas of the UI that have the tap to copy functionality.
status: ready