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.
Details
- Reviewers
tomek atul • abosh - Commits
- rCOMMa9cdb34019a1: [web] Notifications dialog - add text
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
- Branch
- arcpatch-D5281
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
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 ↗ | (On Diff #17280) | 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 ↗ | (On Diff #17280) | Can we add a newline before modelContent to help separate things out? |
Nits inline
web/modals/threads/notifications/notifications-modal.react.js | ||
---|---|---|
206 ↗ | (On Diff #17280) | Can we use a Unicode apostrophe character? |
220 ↗ | (On Diff #17280) | Personally I prefer to avoid ternaries here, see reasoning |
229–233 ↗ | (On Diff #17280) | I don't mind this ternary although technically the doc above is against it |
web/modals/threads/notifications/notifications-modal.react.js | ||
---|---|---|
205 | We should try to keep lines length below 80 characters |