- Notifications
You must be signed in to change notification settings - Fork926
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 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
| queryClient.setQueryData( | ||
| workspaceByOwnerAndNameKey(workspace.owner_name, workspace.name), | ||
| { ...workspace, favorite: !workspace.favorite }, | ||
| ); |
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.
| } | ||
| export const FavoriteButton: FC<FavoriteButtonProps> = ({ | ||
| handleAction, |
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.
It's an implicit convention, but when dealing with event handlers, we usually name them using theon prefix likeonSomething instead ofhandleSomething. So instead ofhandleActions, I think a better name would beonToggle.
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 comment
The reason will be displayed to describe this comment to others.Learn more.
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 puttinguseMutation inside a component.
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.
Created#11892 to follow-up.
| startIcon={isFavorite ? <Star /> : <StarBorder />} | ||
| onClick={() => handleAction(workspaceID)} | ||
| > | ||
| {isFavorite ? "Unfavorite" : "Favorite"} |
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.
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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.
I think you did a really good job on this 🙏. I just left some comments but any of them are blockers.
@johnstcn don't forget to review things in Chromatic 🙏 |
Uh oh!
There was an error while loading.Please reload this page.