Add action on Settings item in thread menu. It displays modal with settings.
Details
Details
Select thread with permission to change settings (like personal thread) and select Settings from thread menu.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 50–52 ↗ | (On Diff #10068) | I know that when you asked me if it's ok to use lambda inside useMemo I said that it is fine, but now I realized it is still risky. |
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 50–52 ↗ | (On Diff #10068) | I'm not sure I follow @palys-swm's hypothetical scenario... since the lambda here depends on both elements in the dep list, there's no way to factor it out in React Hooks. The way Hooks work, onClick must be regenerated every time threadInfo or setModal changes. There are ways to sneak around this with refs or with class components, but in general it's a downside of Hooks that is rarely avoided. That said, I think it would be "good" practice to define onClick as a separate React.useCallback. If somebody later adds another var to the dep list that isn't needed for the onClick callback, and if onClick is getting forwarded onwards to a descendant memoized component (as @palys-swm suggested in his comment), then we'd have the unfortunate effect that a change in the new var that got added to the dep list would cause the descendant memoized component to re-render when it otherwise wouldn't've. |
Comment Actions
Please use callback instead of lambda in onClick - not using it is fine here, but it's safer to use it (see inline comments for details).
| web/chat/thread-menu.react.js | ||
|---|---|---|
| 50–52 ↗ | (On Diff #10068) | The scenario I mentioned describes the case where lambda depends on less values than memo. Also, as you noted, in the future this code might fulfill this scenario, when new dependencies are introduced. So it's just safer to use a callback every time when lambda would be a prop. |