[lib/web] introduce useThreadSettingsNotifications
ClosedPublic
Actions

Authored by ginsu on Jul 3 2024, 1:06 AM.

Details

Reviewers

ashoat
inka

Commits

rCOMM4a83fa595d7c: [lib/web] introduce useThreadSettingsNotifications

Summary

This diff introduces a new hook called useThreadSettingsNotifications which factors out all the shared thread notification settings logic that will be used in both web and native

Test Plan

Confirmed that there were no regresssions with the notifications modal on web

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

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

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

ginsu added reviewers: ashoat, inka.Jul 3 2024, 1:07 AM

ginsu edited the summary of this revision. (Show Details)

ginsu added a parent revision: D12645: [native] extend icon prop in EnumSettingsOption to handle illustration components.

ginsu added a child revision: D12647: [native] introduce barebones UI for ThreadSettingsNotifications.Jul 3 2024, 1:16 AM

Harbormaster completed remote builds in B30075: Diff 41929.Jul 3 2024, 1:22 AM

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

ginsu added inline comments.Jul 3 2024, 1:37 AM

lib/shared/thread-settings-notifications-utils.js
64–74 ↗	(On Diff #41929)	A subsequent diff will handle improving the error handling

inka accepted this revision.Jul 3 2024, 2:39 AM

inka added inline comments.

lib/shared/thread-settings-notifications-utils.js
28 ↗	(On Diff #41929)	This name sounds like a function name, like this variable can be called to disable the save button. Can we call it `saveButtonDisabled`?

This revision is now accepted and ready to land.Jul 3 2024, 2:39 AM

ashoat accepted this revision.Jul 3 2024, 6:52 AM

ashoat added inline comments.

lib/shared/thread-settings-notifications-utils.js
82–85 ↗	(On Diff #41929)	Thanks for adding loading state here
102–104 ↗	(On Diff #41929)	Do we really need all three of these, versus just exposing `setNotificationSettings`, and letting the caller continue creating the callbacks?

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

lib/shared/thread-settings-notifications-utils.js
102–104 ↗	(On Diff #41929)	I guess it's reused in D12647 and so we'd need to be creating the callbacks in two different places. Feel free to ignore this feedback