Skip to content
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

[UI v2] experiment: Adds basic spacing utilities as props and new layout components: Block and Flex #16249

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

devinvillarosa
Copy link
Contributor

@devinvillarosa devinvillarosa commented Dec 6, 2024

Laying out components with raw tailwwind is hard. There are too many options and tailwind classes to pass to simply lay components out. For example, when will I ever use mt-4.5?

This PR experiments in:

  1. Creating a prop utilty and css classes using CVA to be able to pass layout properties as props. These props helps reduce the number of spacing options for the UI so that developers can think less and simplify spacing.

TLDR this PR limits these tailwind properties, provides them as props (which _can _ be added in our base components if needed), and adds <Block /> and <Flex /> components to make laying out components more declarative and reduce decision fatigue on what spacing levels to pass

  1. Create a <Block /> and <Flex /> component that can be used to lay components out using the reduced options mentioned above.

Example

	return (
-		<div className="flex flex-row items-center gap-2">
+		<Flex flexDirection="row" gap={2} alignItems="center">
			<Typography variant="h4">Global Concurrency Limits</Typography>
			<Button onClick={onAdd} size="icon">
				<Icon id="Plus" />
			</Button>
-		</div>
+		</Flex>
	);

The example ☝️ creates a bit more declarative way of laying components out.

Another example

-		<div className="flex flex-row items-center gap-2">
+		<Flex flexDirection="row" gap={2} alignItems="center">
			<Typography variant="h4">Global Concurrency Limits</Typography>
			<Button onClick={onAdd} size="icon">
				<Icon id="Plus" />
			</Button>
-		</div>
+		</Flex>

Checklist

  • This pull request references any related issue by including "closes <link to issue>"
    • If no issue exists and your change is not a small fix, please create an issue first.
  • If this pull request adds new functionality, it includes unit tests that cover the changes
  • If this pull request removes docs files, it includes redirect settings in mint.json.
  • If this pull request adds functions or classes, it includes helpful docstrings.

Related to #15512

| 48
| 64;

export type UtilityProps = Partial<{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to add these props to our components in the future, we can just do something like:

type Props = ButtonProps & UtilityProps
...
return <Button className={cn(buttonVariants({ variant, size, className }), spacingUtiltiesClasses(props))}  ... />

@devinvillarosa devinvillarosa marked this pull request as ready for review December 6, 2024 18:30
ui-v2/src/components/ui/block.tsx Outdated Show resolved Hide resolved
import { createElement, forwardRef } from "react";
import { UtilityProps, spacingUtiltiesClasses } from "./utils/spacing-utils";

type Props = UtilityProps & {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, we might want to omit display here so that this always stays flex.

@devinvillarosa devinvillarosa force-pushed the experiment-spacing-utils branch from 3aa9fa2 to b18afe0 Compare December 6, 2024 19:14
@devinvillarosa devinvillarosa force-pushed the experiment-spacing-utils branch from b18afe0 to 26a2382 Compare December 6, 2024 19:21
Copy link
Member

@desertaxle desertaxle left a comment

Choose a reason for hiding this comment

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

Hooray for readability!

@devinvillarosa devinvillarosa merged commit 9d92835 into main Dec 6, 2024
9 checks passed
@devinvillarosa devinvillarosa deleted the experiment-spacing-utils branch December 6, 2024 19:29
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.

2 participants