[web] Notifications dialog - add text
ClosedPublic
Actions

Authored by michal on Oct 3 2022, 2:32 AM.

Details

Reviewers

tomek
atul
• abosh

Commits

rCOMMa9cdb34019a1: [web] Notifications dialog - add text

Summary

ENG-1729
Adds additional explanation text. For sidebars in the Focused tab, it's displayed under the button and for sidebars in the Background tab it's displayed instead of previous content.

Test Plan

Check that nothing changed for non-sidebars. Check that for each combination of *Parent is in Focused/Background tab* and *can/can't be promoted* for sidebars is displayed correctly.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Oct 3 2022, 2:32 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 3 2022, 2:32 AM

michal added a parent revision: D5280: [web] Notifications dialog - disable background option.Oct 3 2022, 2:32 AM

Harbormaster completed remote builds in B12578: Diff 17280.Oct 3 2022, 2:39 AM

michal requested review of this revision.Oct 3 2022, 2:39 AM

Looks good to me, and looks like @ashoat signed off on the copy in https://linear.app/comm/issue/ENG-1729/update-notifications-dialog-to-handle-threads-correctly.

Adding @tomek as blocking since he's more familiar with sidebar-related stuff.

lib/hooks/promote-sidebar.react.js
34	Can we early exit at the start of `canPromoteSidebar` if the thread type isn't `SIDEBAR`?
web/modals/threads/notifications/notifications-modal.react.js
237	Can we add a newline before `modelContent` to help separate things out?

Nits inline

web/modals/threads/notifications/notifications-modal.react.js
206	Can we use a Unicode apostrophe character?
220	Personally I prefer to avoid ternaries here, see reasoning
229–233	I don't mind this ternary although technically the doc above is against it