Introduce component describing feature of notification settings. It can be true or false (strikethrough text).
Details
Details
Not used in app yet. Tested after introducing notifications modal.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Question inline about distinction between optionFalse, active, etc
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 12 ↗ | (On Diff #12355) | It doesn't look like this div is included in the NotificationsOptoinInfo component? |
| web/modals/threads/notifications/notifications-option-info.react.js | ||
| 17–23 ↗ | (On Diff #12355) | Little confused what is happening here. It looks like we're always going to go with the css.optionInfo selector and we're going with css.optionInfoFalse (which sets text-decoration to strikethrough) if the active prop is false. Is active referring to the "optionInfo" being false? Or the outer notification option being selected? Because it seems like the purpose of div.optionContainerActive was to color the div.optionInfoFalse text differently based on the value of active? Would appreciate some clarification on the naming here |
| 27–30 ↗ | (On Diff #12355) | I think we can use a self-closing tag here |
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 12 ↗ | (On Diff #12355) | Yes, this css should probably be introduced in the next diff, when parent component is introduced. I'll however leave it here in this commit if it's not a problem. |
| web/modals/threads/notifications/notifications-option-info.react.js | ||
| 17–23 ↗ | (On Diff #12355) | Yes, the naming I used here, is really misleading. Sorry for that. I'm planning to change the "outer" active value to selected - to indicate the notification settings option is selected and highlighted. The "active" prop here should also probably be renamed. Maybe it would be better to name it valid to indicate that text is true or feature is activated? |
| 27–30 ↗ | (On Diff #12355) | Thanks for catching it |
Comment Actions
Looks good!
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 12 ↗ | (On Diff #12355) | Gotcha, yeah I think it's fine to include in this diff alongside the "non-optionContainerActive" version of optionInfoFalse since they're related even if it's not used quite yet. |
| web/modals/threads/notifications/notifications-option-info.react.js | ||
| 17–23 ↗ | (On Diff #12355) | Nice, I think using selected/valid would make things clearer |
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 7 ↗ | (On Diff #12433) | sure! |