Page MenuHomePhabricator

[web] Make `valid` prop of `EnumSettingsOptionInfo` optional
AbandonedPublic

Authored by atul on May 15 2022, 11:33 AM.
Tags
None
Referenced Files
F3119583: D4047.diff
Fri, Nov 1, 2:25 AM
Unknown Object (File)
Mon, Oct 28, 6:52 PM
Unknown Object (File)
Mon, Oct 21, 3:57 PM
Unknown Object (File)
Mon, Oct 14, 8:39 AM
Unknown Object (File)
Mon, Oct 14, 8:39 AM
Unknown Object (File)
Mon, Oct 14, 8:34 AM
Unknown Object (File)
Sep 24 2024, 6:45 PM
Unknown Object (File)
Sep 11 2024, 4:34 AM

Details

Summary

Make valid prop of EnumSettingsOptionInfo optional.

Before, when this component was only being used by NotificationsModal, it was being passed description which was an array of [text, isValid] tuples.

Based on the isValid value, the text statement would be styled as being factual or false.

However, when we use this component elsewhere--for example in ThreadSettingsPrivacyTab, we're not going to have a list of true/false statements. Rather, we're going to have a brief text description.

This diff lets us include that brief text description without the accompanying styling (icon + strikethrough (or absense of strikethrough)).

In subsequent diffs EnumSettingsOption will be refactored further to make things cleaner. Specifically, changing the description type and variable names.


Depends on D4046

Test Plan
  1. Manually modify description (array of tuples) such that the second value is undefined/null.
  2. Observe that the EnumSettingsOption appears as expected without check/cross icon or any strikethroughs.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.May 15 2022, 11:34 AM
atul added inline comments.
web/modals/threads/notifications/enum-settings-option-info.react.js
20

implicitly

valid !== undefined && valid != null && valid === false
tomek requested changes to this revision.May 17 2022, 6:14 AM
tomek added inline comments.
web/modals/threads/notifications/enum-settings-option-info.react.js
25–28

I don't think this is a good idea to encode three states in a boolean. We had two states: true and false and now we introduce the third one null / undefined which looks significantly different. A better idea might be to introduce e.g. variant or withIcon flag, or something else.

This revision now requires changes to proceed.May 17 2022, 6:14 AM
atul added inline comments.
web/modals/threads/notifications/enum-settings-option-info.react.js
25–28

I don't think this is a good idea to encode three states in a boolean.

Yeah, you're definitely right. We should determine this explicitly rather than as a "side effect" of a nullable boolean.

A better idea might be to introduce e.g. variant or withIcon flag, or something else.

That makes sense. Going to abandon this diff and put up 2-3 diffs that introduce withIcon flag