Skip to content

Replace all buttons with shadcn buttons #65

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

tstirrat15
Copy link
Contributor

Description

Part of migrating off of Material UI. This seemed like a piece of work that was both big enough and small enough.

Changes

Will annotate.

Testing

Review. See that buttons look fine and work as expected.

Copy link

vercel bot commented May 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 27, 2025 10:49pm

Copy link
Contributor Author

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@@ -268,22 +267,6 @@ const useStyles = makeStyles((theme: Theme) =>
height: "60vh",
width: "100%",
},
hideTextOnMed: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replaced by className="hidden md:inline" on the spans that wrap the text.

@@ -11,8 +11,7 @@ import { useZedTerminalService } from "../spicedb-common/services/zedterminalser
import { parseValidationYAML } from "../spicedb-common/validationfileformat";
import { LinearProgress, Tab, Tabs, Tooltip } from "@material-ui/core";
import AppBar from "@material-ui/core/AppBar";
import Button from "@material-ui/core/Button";
import ButtonGroup from "@material-ui/core/ButtonGroup";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to remove these - I think it looks fine with just the buttons as they are.

@@ -295,18 +278,6 @@ const useStyles = makeStyles((theme: Theme) =>
gridTemplateColumns: "1fr auto",
alignItems: "center",
},
btnAccept: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced these with variants

size="small"
href={AppConfig().discord.inviteUrl}
startIcon={
<Button asChild variant="link" size="sm">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asChild retains the styling of a button but renders the child as the primary element, which makes links behave as expected.

the link variant gives some nice styling. I don't think we should use this for all links, but it looks nice and works well for the buttonlinks we've got.

<a
href={AppConfig().discord.inviteUrl}
target="_blank"
rel="noreferrer"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noreferrer isn't necessary but is generally recommended when opening new tabs. It prevents the target tab from gaining context about the parent.

className={classes.hideTextOnMed}
size="small"
href={AppConfig().discord.inviteUrl}
startIcon={
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startIcon is moot - you just put it as the first child of a shadcn button.

Comment on lines +894 to +900
[
DataStoreItemKind.ASSERTIONS,
DataStoreItemKind.EXPECTED_RELATIONS,
].includes(currentItem.kind) && (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified the conditional to be this rather than separate conditionals that render the same thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant