-
Notifications
You must be signed in to change notification settings - Fork 894
feat(site): show favorite workspaces in ui #11875
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
1848b88
67bef0c
dcb61c8
d4b806d
279e242
a1887ca
279e87f
c46832c
32f6f5f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ import OutlinedBlockIcon from "@mui/icons-material/BlockOutlined"; | |
import PowerSettingsNewIcon from "@mui/icons-material/PowerSettingsNew"; | ||
import RetryIcon from "@mui/icons-material/BuildOutlined"; | ||
import RetryDebugIcon from "@mui/icons-material/BugReportOutlined"; | ||
import Star from "@mui/icons-material/Star"; | ||
import StarBorder from "@mui/icons-material/StarBorder"; | ||
import { type FC } from "react"; | ||
import type { Workspace, WorkspaceBuildParameter } from "api/typesGenerated"; | ||
import { BuildParametersPopover } from "./BuildParametersPopover"; | ||
|
@@ -190,3 +192,24 @@ export const RetryButton: FC<RetryButtonProps> = ({ | |
</TopbarButton> | ||
); | ||
}; | ||
|
||
interface FavoriteButtonProps { | ||
handleAction: (workspaceID: string) => void; | ||
workspaceID: string; | ||
isFavorite: boolean; | ||
} | ||
|
||
export const FavoriteButton: FC<FavoriteButtonProps> = ({ | ||
handleAction, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's an implicit convention, but when dealing with event handlers, we usually name them using the Also, you can move the query inside this button 😁. We're trying to put things close to where they're used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not quite sure how to accomplish this; React doesn't seem to like me putting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Created #11892 to follow-up. |
||
workspaceID, | ||
isFavorite, | ||
}) => { | ||
return ( | ||
<TopbarButton | ||
startIcon={isFavorite ? <Star /> : <StarBorder />} | ||
onClick={() => handleAction(workspaceID)} | ||
> | ||
{isFavorite ? "Unfavorite" : "Favorite"} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this button can have two states, favorite and unfavorite, I would create a story in a storybook for both scenarios. |
||
</TopbarButton> | ||
); | ||
}; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One small thing about the design is that I don't think it should be a primary action, located in the same place as workspace actions like start, stop, refresh, etc. I think it should be in the "more options" menu. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think @matifali ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm. I like it on the front page, but what about making a filled ★ vs. an empty ✰ star only and removing the text altogether? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer to leave the text on, as a button should show what will happen when you click it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @BrunoQuaresma I think we have enough space to leave the button up top for now; hiding it in the menu makes it less discoverable. |
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.
You also may want to invalidate the "workspace list" query data.