refactor: Move schedule to the header #2775
Conversation
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
site/src/components/WorkspaceScheduleButton/WorkspaceScheduleButton.tsx
Outdated
Show resolved
Hide resolved
|
I know I left a lot of comments but I think this is looking really spiffy. Love the redesign. |
| // so to avoid doing that every seconds - because of the workspace pooling - we | ||
| // are memoizing this component. More details: | ||
| // https://github.com/coder/coder/pull/2775#discussion_r912080377 | ||
| const MemoizedWorkspaceScheduleButton = React.memo(WorkspaceScheduleButton) |
There was a problem hiding this comment.
@BrunoQuaresma I haven't used memo before - only useMemo - so this is new to me. Does this somehow update the Button component if the workspace changes? Also I know we are relying on Dayjs() in the component - we probably don't want the current moment to memoized, right? Otherwise it will get stale. IDK, maybe I'm misunderstanding how this works.
There was a problem hiding this comment.
Yeah, looks like maybe the workspace will update the component because it is a prop, but not sure that the current time will get updated, so our comparisons might be off.
There was a problem hiding this comment.
Thinking better, memoization for this component does not solve anything since the workspace prop reference will change every second. useMemo also would not work directly, since I would have to pass the workspace to the deps array, and like before, the reference of this object changes every 1 second. Probably I think we can solve that by writing a custom comparison function but I really would like to avoid that until it is needed. Since it looks a bit more complex, is it ok to not have memoization at all for now?
There was a problem hiding this comment.
Yeah, that's fair! Thanks for trying it out; I appreciate it!
Closes #2749
You can better see it on Chromatic!