Introduce modal component for notifications settings in thread. Currently it doesn't have "onSave" callback, and the notifications settings itself.
The settings options will be added in separate diff.
Details
Details
It is not accesible from menu yet. Can be launched after diff introducing menu item in thread menu.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Adding @ashoat as this diff touches the copy (modal name and save button)
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 7 ↗ | (On Diff #12360) | If we use the same value in a couple of places, maybe we should extract it to common place? If we're using a different value every time, maybe we should make it more consistent? |
| web/modals/threads/notifications/notifications-modal.react.js | ||
| 24–32 ↗ | (On Diff #12360) | Maybe like this? |
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 7 ↗ | (On Diff #12360) | Why do we need to hard-code a min-width value here? The value seems particularly specific.. so I'm guessing it's from the Figma designs? |
| web/modals/threads/notifications/notifications-modal.react.js | ||
| 12–15 ↗ | (On Diff #12360) | I think(?) we generally put the "Props" type right above the component. So maybe swap positions with the NotificationSettings type? Don't feel strongly, nor do I know if this is something we care about keeping consistent... but figured I'd throw it out there |
| 24 ↗ | (On Diff #12360) | Maybe like initialThreadSetting? (don't feel strongly, feel free to go with whatever) |
| 36 ↗ | (On Diff #12360) | Thanks for the heads up |
| web/modals/threads/notifications/notifications-modal.css | ||
|---|---|---|
| 7 ↗ | (On Diff #12360) | This min-width value is used to make the modal match size with design in Figma. The value is specific for each modal, although some of them have the same size. As the design system doesn't define a common size of modals, I'm not sure if we should extract the value. |
| web/modals/threads/notifications/notifications-modal.react.js | ||
| 12–15 ↗ | (On Diff #12360) | Yes, I agree with you |
| 24 ↗ | (On Diff #12360) | It's much better name, thanks for suggestion |
| 24–32 ↗ | (On Diff #12360) | Yes, it looks better. Thanks! |