Details

Reviewers

ashoat
inka

Commits

rCOMMeb5b3d8405ec: [native] introduce NotificationDescription

Summary

This diff introduces a small component called NotificationDescription that handles rendering the descriptions of each notification setting option. We also in this diff need to extend the description prop in EnumSettingOption to be able to handle NotificationDescription

Depends on D12647

Test Plan

Please see the demo video:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ginsu created this revision.Jul 3 2024, 1:18 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 3 2024, 1:18 AM

ginsu edited the summary of this revision. (Show Details)Jul 3 2024, 1:24 AM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: ashoat, inka.

ginsu added a parent revision: D12647: [native] introduce barebones UI for ThreadSettingsNotifications.

ginsu attached a referenced file: F2174884: Screenshot 2024-07-03 at 4.23.24 AM.png. (Show Details)

ginsu attached a referenced file: F2174885: Screenshot 2024-07-03 at 4.23.33 AM.png. (Show Details)

ginsu attached a referenced file: F2174879: Screenshot 2024-07-03 at 4.23.14 AM.png. (Show Details)

Harbormaster completed remote builds in B30077: Diff 41931.Jul 3 2024, 1:34 AM

ginsu requested review of this revision.Jul 3 2024, 1:34 AM

inka added inline comments.Jul 3 2024, 3:28 AM

native/components/enum-settings-option.react.js
115 ↗	(On Diff #41931)	The style changes change now the element looks in my setup. But I think it's fine without changes: with changes:

ashoat added inline comments.Jul 3 2024, 6:41 AM

native/components/enum-settings-option.react.js
115 ↗	(On Diff #41931)	Hey @inka I think your images didn't attach

inka attached a referenced file: F2175835: image.png. (Show Details)Jul 3 2024, 6:43 AM

inka attached a referenced file: F2175838: image.png. (Show Details)

inka added inline comments.

native/components/enum-settings-option.react.js
115 ↗	(On Diff #41931)	Thank you!

ashoat added inline comments.Jul 3 2024, 6:57 AM

native/components/enum-settings-option.react.js
115 ↗	(On Diff #41931)	Agree it looked better before, when the icon was alligned with the "Announcement root" title

Passing back mainly for @inka's feedback

native/chat/settings/thread-settings-notifications.react.js
77 ↗	(On Diff #41931)	Is it worth factoring out the copy-pasted strings between `web`/`native`?
89 ↗	(On Diff #41931)	Can we actually say "Notif count" here, and update the original place it was copied from? Got some feedback from Android users that this is confusing
98 ↗	(On Diff #41931)	You have a task assigned to rename these, but I'm guessing you're planning on handling all of the callsites at once rather than updating these ones first