Page MenuHomePhabricator

[web] Replace `EnumSettingsOption` `description` prop with `statements` prop
ClosedPublic

Authored by atul on May 15 2022, 12:02 PM.
Tags
None
Referenced Files
F3363191: D4048.diff
Mon, Nov 25, 12:46 AM
Unknown Object (File)
Sat, Nov 9, 8:24 PM
Unknown Object (File)
Sat, Nov 9, 7:05 PM
Unknown Object (File)
Sat, Nov 9, 4:59 PM
Unknown Object (File)
Sat, Nov 9, 4:58 PM
Unknown Object (File)
Sat, Nov 9, 3:18 PM
Unknown Object (File)
Sat, Nov 9, 2:58 PM
Unknown Object (File)
Sat, Nov 9, 10:40 AM

Details

Summary

description was a $ReadOnlyArray<[string, boolean]> with a statement and a boolean indicating whether the statement was true.

Replaced description with statements which has type: $ReadOnlyArray<{statement: string, isStatementValid: boolean}>

I think this is a more accurate prop name, since description makes me think we have a continuous blob of text when we really have discrete statements and maybe (depending on whether isStatementValid isn't null/undefined) some associated styling to indicate whether the statement is true or false.

Also explicitly named and typed the contents of the $ReadOnlyArray so things are a bit more "self-documenting"


Depends on D4047

Test Plan
  1. Opened the NotificationsModal
  2. Ensured that things worked and looked as expected.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.May 15 2022, 12:08 PM
web/modals/threads/notifications/enum-settings-option.react.js
16–17 ↗(On Diff #12693)

These properties should probably be $ReadOnly

atul marked an inline comment as done.

make statements.statement and statements.isStatementValid $ReadOnly

web/modals/threads/notifications/enum-settings-option.react.js
16–17 ↗(On Diff #12693)

thanks for catching that, updated the diff

tomek added inline comments.
web/modals/threads/notifications/notifications-modal.react.js
119

It might be a good idea to create constants with these strings so that we're sure they are consistent

This revision is now accepted and ready to land.May 17 2022, 6:16 AM

rebase after resolving merge conflicts and before landing

web/modals/threads/notifications/notifications-modal.react.js
119

Makes sense, will address in followup diff