feat: new text message (1) #15005
feat: new text message (1) #15005
Conversation
Jenkins BuildsClick to see older builds (36)
|
| :padding-left 10 | ||
| :border-left-color colors/neutral-40}) | ||
|
|
||
| (def code |
|
|
||
| (defn text-content | ||
| [message-data context] | ||
| [rn/view | ||
| [old-message/render-parsed-text message-data] | ||
| (render-parsed-text message-data) |
There was a problem hiding this comment.
( most common mistake in reagent, probably we need to add this to guidelines, cc @ilmotta
There was a problem hiding this comment.
it should be [render-parsed-text message-data]
There was a problem hiding this comment.
This problem you're describing is extremely important I agree, and it's well documented by Reagent and everybody should know. It even has a separate file in their docs https://github.com/reagent-project/reagent/blob/master/doc/UsingSquareBracketsInsteadOfParens.md
I believe this mistake is getting into the territory of "tech stuff everybody should know in status-mobile", so it's not really a guideline to me. Devs need to understand why parentheses is not okay, otherwise they'll make the same mistakes again.
Going on a tangent, have you seen https://github.com/roman01la/uix? It could be an interesting improvement over Reagent. It interprets hiccup much faster than Reagent, and it's still compatible with Reagent. Could be interesting to compare two screens (on written in Reagent and another one using uix) in mobile to see if there's any meaningful performance difference.
There was a problem hiding this comment.
But after re-reading that official doc I see there's a strong recommendation to use [], but the author gives a bit of room for interpretation. I retract what I said, I'll add to the guidelines as you suggested @flexsurfer
There was a problem hiding this comment.
Just created new PR with this new recommendation #15008
| [from] | ||
| (rf/sub [:contacts/contact-name-by-identity from])) | ||
|
|
||
| (defn render-inline |
There was a problem hiding this comment.
this looks like a copy from old code? or do we really have all these in new designs ?
There was a problem hiding this comment.
i believe there is no status tag anymore, also code doesn't look implemented as in designs , not sure about "emph" "strong" "strong-emph" and "del" whats that, would be cool to have table in the description with the screenshot for each case
There was a problem hiding this comment.
@flexsurfer Removed the status tag. Here is the table.
Emph is italic, strong is bold, del is strikethrough. These differences in names comes from status-go.
Not all cases are implemented yet. There is a second issue: #15004
There was a problem hiding this comment.
I believe it's not in the designs, but #community-channel should be supported, @John-44 @pedro-et is that correct?
@OmarBasem if that's the case, would you mind creating an issue tracking this please?
There was a problem hiding this comment.
@cammellos do you mean the status-tag? There is a second issue, I can update it to include that also: #15004
There was a problem hiding this comment.
@OmarBasem yes, I believe I had recently a discussion with @John-44 / @pedro-et and they didn't mention that we were to remove them, @ibrkhalil has also done some work on it recently (yet unmerged), so I think we will support them (I think desktop does, but I could be wrong), would you mind checking with them directly please?
There was a problem hiding this comment.
Sure, I will check them 👍
There was a problem hiding this comment.
Got an approval from Pedro that we should support #links to redirect to community channels #15069
There was a problem hiding this comment.
Will add them on my PR
|
|
||
| (defn mention-element | ||
| [from] | ||
| (rf/sub [:contacts/contact-name-by-identity from])) |
There was a problem hiding this comment.
What's the point of this mention-element function? It's currently sort of just "aliasing" the subscription, but there's no hiccup being built.
| (defn render-inline | ||
| [acc {:keys [type literal destination]}] | ||
| (case type | ||
| "" |
There was a problem hiding this comment.
The "empty type" case is the same as the default one, so this first check can be safely eliminated.
It's also a tad bit more idiomatic to convert the type to a keyword to do the matching.
(case (keyword type)
:code
(conj acc [rn/view {:style (merge style/block (style/code))} [quo/text {:weight :code} literal]])
:emph
(conj acc [quo/text {:style {:font-style :italic}} literal])
...)| (defn render-block | ||
| [acc {:keys [type ^js literal children]}] | ||
| (case type | ||
|
|
| "codeblock" | ||
| (conj acc | ||
| [rn/view {:style (merge style/block (style/code))} | ||
| [quo/text (.substring literal 0 (dec (.-length literal)))]]) |
There was a problem hiding this comment.
I understand this is coming from the original code, but since it's open for review and it's quick to change I'll mention: we should use subs instead of substring and count instead of length.
I actually don't understand why literal has a type hint. Every native string in Javascript is automatically supported by ClojureScript.
Isn't literal just a string or did I get it incorrectly?
|
|
||
|
|
||
| (defn render-block | ||
| [acc {:keys [type ^js literal children]}] |
There was a problem hiding this comment.
I understand this function is used in a reducing context in the function render-parsed-text, but this doesn't mean the name acc should carry over here because this function doesn't need to know it's used in a reducing function. acc could be renamed to blocks instead.
I have the same feedback for the function render-inline. acc in that function should be renamed to something better.
acc only makes sense when co-located with the reduce call, they are like bread and butter, but only when close together.
| "paragraph" | ||
| (conj acc | ||
| (reduce | ||
| (fn [acc e] (render-inline acc e)) |
There was a problem hiding this comment.
It's unnecessary to create an anonymous function. You can just use (reduce render-inline).
|
|
||
| (defn render-parsed-text | ||
| [{:keys [content]}] | ||
| (reduce (fn [acc e] |
There was a problem hiding this comment.
It's unnecessary to create an anonymous function. You can just use (reduce render-inline).
- New section: Use [] instead of () in Reagent components: New section after this comment #15005 (comment) - Updated section "Don't use percents to define width/height": reworded to follow the style of the rest of the document. - Update section "Accessibility labels": added example about avoiding dynamic labels.
OmarBasem
force-pushed
the
new-text-message
branch
from
2ec7c6d
to
32f0a73
Compare
OmarBasem
force-pushed
the
new-text-message
branch
from
32f0a73
to
bb31666
Compare
69% of end-end tests have passedFailed tests (8)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (18)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
OmarBasem
force-pushed
the
new-text-message
branch
from
bb31666
to
50cde07
Compare
OmarBasem
force-pushed
the
new-text-message
branch
2 times, most recently
from
58323b9
to
a1d7417
Compare
OmarBasem
force-pushed
the
new-text-message
branch
from
a1d7417
to
eba86e8
Compare
|
Hey QA team @VladimrLitvinenko @pavloburykh @qoqobolo, can I get a review please? |
Hi @OmarBasem thank you for PR. Please note that the whole QA team is now busy testing the release, so PRs not included in the rc1 scope and will be tested after the release testing is completed. |
61% of end-end tests have passedNot executed tests (8)Failed tests (7)Click to expandClass TestActivityCenterMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (11)Click to expandClass TestActivityCenterMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
Hi @OmarBasem I tried to investigate to figure out the changes that were done in this PR toward the styling mentions, links, and text formatting
Did I understand correctly that it should be shown as in the design now?
|
OmarBasem
force-pushed
the
new-text-message
branch
from
74809f8
to
21e9af5
Compare
|
Hi @VladimrLitvinenko, visually the changes in this PR are minor. Fixed the underlining in links. Addresses styling is not part of this PR. There is a second issue: #15004 |
65% of end-end tests have passedFailed tests (9)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeeplinkOneDeviceNewUI:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (17)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
@OmarBasem thank you for PR. The Issue with the link underlining is fixed. I catch only one strange issue which appears only when the email added from my android keyboard hints. Maybe you can see what going with help of logs ISSUE 1: <Cannot read property 'substring' of null> error is shown when the email domain is added from the keyboard hintsSteps to reproduce:
Actual result: error.mp4Expected result: Logs: Env: Huawei p20 light, android version 9 |
|
@VladimrLitvinenko that does not seem to be related to this PR. It is a problem with the input. Feel free to open an issue. |
Hi, @OmarBasem tried reproducing this issue again to see if it's in development. But it is not reproducible. It looks like this error is only related to this PR. Could you fix it in the scope of this PR? |
status-im/status-go@550de3b...550de3b updates test updates rebase rebase rebase status-im/status-go@550de3b...550de3b rebase status-im/status-go@550de3b...550de3b new text message status-im/status-go@550de3b...550de3b feat: new text message feat: new text message
|
Thank you @OmarBasem! It looks like this issue is an OS edge case. Could you merge this PR, please? |
OmarBasem
force-pushed
the
new-text-message
branch
from
21e9af5
to
9f7a3d0
Compare
|
Thanks @VladimrLitvinenko |
fixes: #14975
This PR implements a new text message component with support for styling mentions, links, and basic text formatting.
Note for QA:
Not all formattings are yet supported. Second issue: #15004