Quo2: fix reaction styles #17224
Quo2: fix reaction styles #17224
Conversation
Jenkins BuildsClick to see older builds (20)
|
| @@ -12,31 +12,32 @@ | |||
| [rn/touchable-opacity | |||
There was a problem hiding this comment.
Nit: the name space of this component is wrong - it's not to do with your changes but perhaps while you are working on these files you can migrate it to the correct location?
I.e these should be in .selectors. To match figma and the name should match that of figma.
There was a problem hiding this comment.
Additionally these files should only export one component and it seems like this file has two in it, can we adjust that and map with fi hmm a 1-1 as we have in the rest of our components? Makes things easier to maintain and to understand what we have in place etc 🙂
| :flex-direction :row | ||
| :align-items :center} | ||
| [quo2.reaction/reaction @state] | ||
| [quo2.reaction/add-reaction @state]]]]))) | ||
| [quo/reaction @state] |
There was a problem hiding this comment.
Same for these preview screens, we should only have one design system component per preview screen- could you fix this here or as a follow up? 🙏
There was a problem hiding this comment.
was thinking the same. I think as a follow-up might be better, cause the component also seems incomplete (is missing a prop from the DS)
There was a problem hiding this comment.
Sounds great, maybe we can some simple unit tests in that too and align some other best practices :)
67% of end-end tests have passedFailed tests (14)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (29)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
Ready for design review cc @Francesca-G |
There was a problem hiding this comment.
The component looks good when tested in a chat, reacting to a message but when I open it from the Quo2 Preview -> selectors -> select-reactions it looks different:
is this supposed to happen?
(Also, once I opened it I had to close the application, because there was no back button in the preview).
Moreover, we should discuss with Pedro and the rest of the team about a solution for the emoji library. We're supposed to use twemoji, so the component is correct, but since we're using system emoji for the time being we should adapt the component to the system ones, to be consistent. Otherwise we will have reactions with twemoji + system emojis in messages.
@OmarBasem - this sounds related to recent nav bar changes, do you know if that's all working fine?
is this supposed to happen?
(Also, once I opened it I had to close the application, because there was no back button in the preview).
with respect to twemoji etc @smohamedjavid has researched this area a lot recently and can probably provide some input on this? 🤨
|
@Francesca-G Sorry, didn't add the exact location in the preview. The component was originally located in the wrong place (under |
|
@clauxx sure, adding the follow up required tag |
We need to update the preview screen for reactions (
@Francesca-G - The team decided to defer Twemoji integration in the mobile. It will be implemented post-MVP. We will be using the default emojis from the OS (Android or iOS) until then. |
clauxx
force-pushed
the
16907-fix-reaction-styles
branch
from
8185e86
to
3af0daf
Compare
Ok good, so we should adapt the react component so that it aligns with the default emojis and we have a consistent style throughout :) |
I believe the Twemoji (and default emojis) is not applicable to the reactions as the reaction emojis are unique and limited to 6. These emojis are the same as the desktop despite the desktop uses Twemoji. |
|
@smohamedjavid as mentioned by Pedro, ideally the reaction emojis should be the same as what the user can send in messages, so we should adapt this component to OS emojis. Is that possible? |
It's possible. We need to remove the existing custom reaction images/emojis and add the equivalent default OS emoji for the reaction. BTW this is a completely new change and requires separate testing effort. We will need a separate issue to track it. cc: @flexsurfer |
|
@cammellos @J-Son89 can you please merge it? I'm not authorized 😢 |
The file |
Can take care of that in the follow-up issue |
flexsurfer
force-pushed
the
16907-fix-reaction-styles
branch
from
2fa3097
to
7bfb341
Compare
clauxx
force-pushed
the
16907-fix-reaction-styles
branch
from
7bfb341
to
6842223
Compare
fixes #16907
Summary
Based on the following feedback comment, the size of the reaction and add-reaction components were off from the designs. Additionally, fixed the colors based on the Design System component.
Testing notes
Use Caseproperty from the Design System is not included in the component. Could be added as a follow up issue.Platforms
Steps to test
Previewmenustatus: ready