feat(site): move history into sidebar #11413
Conversation
|
Ran out of time to start on this today, but I'll be starting on this first thing tomorrow morning! |
| return <Link css={styles.sidebarItem} {...props} />; | ||
| }; | ||
|
|
||
| export const SidebarItem = (props: HTMLAttributes<HTMLButtonElement>) => { |
There was a problem hiding this comment.
Just for future-proofing, do you think it's worth explicitly pulling the type attribute out and setting its default value to button?
There was a problem hiding this comment.
Not sure if I got it... do you have a code sample?
There was a problem hiding this comment.
Sorry, just realized that the comment got garbled by a stray backtick
Now that I'm a little more awake, I think my suggestion would be overkill, but I was talking about this:
export const SidebarItem = ({
type = "button",
...delegatedProps
}: HTMLAttributes<HTMLButtonElement>) => {
return <button css={styles.sidebarItem} type={type} {...props} />;
};| <div css={{ padding: 16 }}> | ||
| <LoadingButton | ||
| fullWidth | ||
| onClick={() => buildsQuery.fetchNextPage()} |
There was a problem hiding this comment.
Could this be simplified to onClick={buildsQuery.fetchNextPage}?
There was a problem hiding this comment.
No, because the onClick event pass an Event object and the fetchNextPage receives a different argument type
| // Watch workspace changes | ||
| const updateWorkspaceData = useEffectEvent( | ||
| async (newWorkspaceData: Workspace) => { | ||
| if (!workspace) { |
There was a problem hiding this comment.
Maybe it's just because it's still early for me, but I got confused for a second when reading this code because I didn't realize thought the check was for the old workspace, and not the new one
Do you think it's worth renaming workspace to currentWorkspace, to make the difference between new and old a little more clear?
There was a problem hiding this comment.
This workspace is a prop from the WorkspacePage so I don't think renaming it would make it look good at the component level. Eg. <WorkspacePage currentWorkspace={workspace} /> does not make too much sense to me.
|
|
||
| {buildLogs} | ||
|
|
||
| {typeof resources !== "undefined" && resources.length > 0 && ( |
There was a problem hiding this comment.
Could the condition be simplified to resources?.length > 0?
There was a problem hiding this comment.
No, because typescript will throw an error:
But it can be something like resources && resources.length > 0
There was a problem hiding this comment.
Oh, that's my mistake – this is one of those things that's technically safe in JavaScript (undefined > 0 evaluates to false), but TS wouldn't be happy
|
Related to #11491 |
Screen.Recording.2024-01-04.at.13.50.39.mov