feat(site): show favorite workspaces in ui #11875
Conversation
johnstcn
commented
Jan 29, 2024
•
| queryClient.setQueryData( | ||
| workspaceByOwnerAndNameKey(workspace.owner_name, workspace.name), | ||
| { ...workspace, favorite: !workspace.favorite }, | ||
| ); |
There was a problem hiding this comment.
You also may want to invalidate the "workspace list" query data.
| } | ||
|
|
||
| export const FavoriteButton: FC<FavoriteButtonProps> = ({ | ||
| handleAction, |
There was a problem hiding this comment.
It's an implicit convention, but when dealing with event handlers, we usually name them using the on prefix like onSomething instead of handleSomething. So instead of handleActions, I think a better name would be onToggle.
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.
Also, you can move the query inside this button 😁. We're trying to put things close to where they're used.
I'm not quite sure how to accomplish this; React doesn't seem to like me putting useMutation inside a component.
There was a problem hiding this comment.
Created #11892 to follow-up.
| startIcon={isFavorite ? <Star /> : <StarBorder />} | ||
| onClick={() => handleAction(workspaceID)} | ||
| > | ||
| {isFavorite ? "Unfavorite" : "Favorite"} |
There was a problem hiding this comment.
Since this button can have two states, favorite and unfavorite, I would create a story in a storybook for both scenarios.
There was a problem hiding this comment.
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.
What do you think @matifali ?
There was a problem hiding this comment.
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.
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.
@BrunoQuaresma I think we have enough space to leave the button up top for now; hiding it in the menu makes it less discoverable.
|
@johnstcn don't forget to review things in Chromatic 🙏 |