- Notifications
You must be signed in to change notification settings - Fork925
feat(site): move history into sidebar#11413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Ran out of time to start on this today, but I'll be starting on this first thing tomorrow morning! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks really good! Just had a few niggling comments
Uh oh!
There was an error while loading.Please reload this page.
| return <Link css={styles.sidebarItem} {...props} />; | ||
| }; | ||
| export const SidebarItem = (props: HTMLAttributes<HTMLButtonElement>) => { |
ParkreinerJan 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just for future-proofing, do you think it's worth explicitly pulling thetype attribute out and setting its default value tobutton?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if I got it... do you have a code sample?
ParkreinerJan 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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:
exportconstSidebarItem=({ type="button", ...delegatedProps}:HTMLAttributes<HTMLButtonElement>)=>{return<buttoncss={styles.sidebarItem}type={type}{...props}/>;};
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| <div css={{ padding: 16 }}> | ||
| <LoadingButton | ||
| fullWidth | ||
| onClick={() => buildsQuery.fetchNextPage()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could this be simplified toonClick={buildsQuery.fetchNextPage}?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, because theonClick event pass an Event object and thefetchNextPage receives a different argument type
| // Watch workspace changes | ||
| const updateWorkspaceData = useEffectEvent( | ||
| async (newWorkspaceData: Workspace) => { | ||
| if (!workspace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 renamingworkspace tocurrentWorkspace, to make the difference between new and old a little more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
| {buildLogs} | ||
| {typeof resources !== "undefined" && resources.length > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could the condition be simplified toresources?.length > 0?
BrunoQuaresmaJan 5, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, because typescript will throw an error:
But it can be something likeresources && resources.length > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh, that's my mistake – this is one of those things that's technically safe in JavaScript (undefined > 0 evaluates tofalse), but TS wouldn't be happy
Related to#11491 |
Screen.Recording.2024-01-04.at.13.50.39.mov