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
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
Unknown Object (File)
Tue, Nov 5, 9:57 PM

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
Branch
landmay19 (branched from master)
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 ↗(On Diff #12764)

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 ↗(On Diff #12764)

Makes sense, will address in followup diff