[#22404] [News Feed] Implement News AC component #22453
Conversation
Jenkins BuildsClick to see older builds (16)
|
flexsurfer
force-pushed
the
feature/News-Feed]-Implement-News-AC-component-#22404
branch
from
5f4729f to
8393351
Compare
flexsurfer
changed the title
There was a problem hiding this comment.
Thank you @flexsurfer. While testing the PR, I realized we are showing these <br> tags, but this looks bad, especially for non-technical users.
I haven't checked how the text transformation is done in status-go, but I believe we should strip the news feed text from HTML tags and deliver it ready for consumption. Well, at least the obvious cases, like br tags should be considered. @jrainville do you know if this can be fixed for this release?
A simple approach that I've seen is to replace each br tag with a newline character. Another approach is to transform the text to simple markdown (no extensions), but this may not be rendered well in mobile.
| @@ -38,6 +39,14 @@ | |||
| (not preview-privacy?)])) | |||
| [preview-privacy?]) | |||
|
|
|||
| toggle-news-feed | |||
| (rn/use-callback (fn [] | |||
| (rf/dispatch [:profile.settings/profile-update :news-feed-enabled? | |||
There was a problem hiding this comment.
Sharing for awareness: it's not the first time I see an opportunity to make settings_saveSetting support multiple settings at once so that we can do one atomic update.
src/status_im/contexts/shell/activity_center/notification/news/style.cljs
Outdated
Show resolved
Hide resolved
| [utils.i18n :as i18n] | ||
| [utils.re-frame :as rf])) | ||
|
|
||
| (defn auto-resized-image |
There was a problem hiding this comment.
We can use custom hooks more often, even if they will be used only once, since a hook is just a function anyway. The following is untested code. This allows us to decouple state management & effects from hiccup.
Note: untested code.
Note 2: I'm passing the url as a dependency to use-effect
(defn- use-auto-image-height
[url]
(let [[height set-height] (rn/use-state nil)
window-width (- (:width (rn/get-window)) 40)]
(rn/use-effect
(fn []
(-> (rn/image-get-size url)
(promesa/then (fn [[w h]]
(let [scale (/ window-width w)
new-height (* h scale)]
(set-height new-height))))))
[url])
{:height height
:width window-width}))
(defn auto-resized-image
[url]
(let [{:keys [height width]} (use-auto-image-height url)]
(if height
[rn/image
{:resize-mode :contain
:style {:width width
:height height
:align-self :center
:border-radius 12}
:source url}]
[rn/view {:style {:height 200 :align-items :center :justify-content :center}}
[rn/activity-indicator]])))
src/status_im/contexts/shell/activity_center/notification/news/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im/contexts/shell/activity_center/notification/news/view.cljs
Outdated
Show resolved
Hide resolved
|
@flexsurfer @xAlisher Some notification types have a default action when pressed. For thew News Feed notifications I think we should also have a default action and make the on-press of the notification open the bottom sheet, so that the user doesn't have to exactly press on "Read more" |
thanks @ilmotta i think it's just a bad test example, i just copy pasted it from our blog, it won't be a case for the real news in production, nobody will use tags there, so we're fine |
Oh in that case sounds great @flexsurfer. The reason I asked this is also because the Discord bridge also has an issue where messages will appear with |
79% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletCollectibles:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
Passed tests (19)Click to expandClass TestWalletCollectibles:
Class TestWalletCustomParamOneDevice:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDeviceTwo:
Class TestProfileMultipleDevices:
Class TestWalletOneDevice:
|
|
Hey @flexsurfer Do we have any documented requirements or guidelines on what types of content are supported in mobile News Feed posts? For example, when creating a post in Ghost.io, I tried testing various content types - images, image galleries, dividers, embedded YouTube videos, toggle headers, markdown formatting, etc. - but none of them rendered properly in the Status app (except text and buttons). I'm wondering if the content creators are aware of these limitations. Is this captured anywhere in documentation or specs? Attaching:
12312423432.mp4telegram-cloud-document-2-5285423377643764090.mp4link to the Post https://status-news-feed-mobile.ghost.io/test-different-types-of-data-in-post/ cc @ilmotta |
|
@pavloburykh in this PR only plain text is supported, not sure about future |
flexsurfer
force-pushed
the
feature/News-Feed]-Implement-News-AC-component-#22404
branch
from
e335e9e to
59323e5
Compare
ISSUE 1 Missing unread dot near unread News notificationSteps:
Actual result: not unread dot
telegram-cloud-document-2-5285423377643764494.mp4Expected result: unread dot is shown 
|
ISSUE 2 Text and feature image disappear on AndroidReproduced on Android 12, Samsung Galaxy A52 Steps:
Examples of posts:
Actual result:
telegram-cloud-document-2-5287498220509884504.mp4 |
|
thanks @pavloburykh ISSUE 1 is fixed |
fixed, also mark notification as read on press |
|
@pavloburykh i would file ISSUE 2 separately with low prio wdyt? |
Hmm… honestly, I’m not sure we can treat this as a low-priority issue. From a user’s perspective, the news notification appears broken - the content disappears - which seems like a critical problem for this feature. That said, if you feel differently, we could check with @ilmotta and get a second opinion. |
|
@pavloburykh @flexsurfer, regarding ISSUE 2.
Does the bug appear in other Android devices besides the Samsung Galaxy? Does it happen in a similar device with another Android version? I understand this can be tricky or time consuming to verify @pavloburykh, but it would help us make the best decision. If it does fail in other devices/OSes, then the problem is quite important to be fixed. If the bug is isolated to just this single Galaxy phone, then the bug could be fixed in a follow-up as @flexsurfer suggested.
From the video, I also got this impression that the bug is actually quite critical because it undermines the basic experience. |
Thanks @ilmotta! The bug is reproducible on Pixel 7a, Android 15 as well (thanks to @Horupa-Olena for verifying). So we need to fix it.
telegram-cloud-document-2-5287551924780956066.mp4cc @flexsurfer |
jakubgs
restored the
feature/News-Feed]-Implement-News-AC-component-#22404
branch
fixes #22404
fixes #22405
fixes #22407
fixes #22408
Screen.Recording.2025-05-16.at.12.55.47.mov
status-go: status-im/status-go#6584