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
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
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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? |
Comment Actions
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 ↗ | (On Diff #17316) | We should try to keep lines length below 80 characters |