feat: add box package #1067
feat: add box package #1067
Conversation
🦋 Changeset detectedLatest commit: eb76380 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +29 B (0%) Total Size: 188 kB
ℹ️ View Unchanged
|
|
I'd prefer to resolve differences between our two proposals before merging either (other proposal for reference). There's a lot here that I like (specifically the style generation from our tokens), but there are a few elements of my branch that could or maybe should be pulled over, too. @Niznikr Can we set up some time early next week to discuss? |
packages/box/package.json
Outdated
| }, | ||
| "dependencies": { | ||
| "@launchpad-ui/vars": "workspace:~", | ||
| "@radix-ui/react-slot": "^1.0.2", |
There was a problem hiding this comment.
Do we typically pin our dependencies?
packages/box/src/Box.tsx
Outdated
| 'data-test-id'?: string; | ||
| }; | ||
|
|
||
| const Box = ({ asChild, children, 'data-test-id': testId = 'box', ...props }: BoxProps) => { |
There was a problem hiding this comment.
Do we need a default test ID, or should we just omit this entirely if it's not provided?
There was a problem hiding this comment.
Perhaps only set it if asChild is not present? We typically set a default test ID for all LP components.
| // Build out utility classes that don't use CSS variables | ||
| display: ['block', 'flex', 'inline-block', 'inline-flex'], | ||
| }, | ||
| shorthands: { |
There was a problem hiding this comment.
@apucacao expressed some concern with the shorthand properties and the legibility of them. Should we just omit them for now? (paddingY/paddingX/marginY/marginX might still make sense from a legibility perspective, though)
| type FillKeys = FlattenObjectKeys<typeof fill>; | ||
| type TextKeys = FlattenObjectKeys<typeof text>; | ||
|
|
||
| const colors = flatten<typeof global, Record<GlobalKeys, string>>(global); |
There was a problem hiding this comment.
A question about how we tokenize these. Do we want to use hyphen - delimiters rather than periods .? That would map more cleanly to how they're expressed in CSS.
There was a problem hiding this comment.
I slightly prefer the periods as it clears up the hierarchy/taxonomy of our tokens in cases where hyphens are used in some names (example).
There was a problem hiding this comment.
Cool, that makes sense to me!
|
|
||
| const colorProperties = defineProperties({ | ||
| conditions: { | ||
| lightMode: { selector: ':root &, [data-theme="default"] &' }, |
There was a problem hiding this comment.
A small thing: Maybe this should just be default and dark, to match the names of the themes as they exist in other parts of the code?
| }, | ||
| }); | ||
|
|
||
| const colorProperties = defineProperties({ |
There was a problem hiding this comment.
Should this be themedProperties and maybe include borderStyle as well? Not sure if that would be considered a themed value or a responsive value 🤔 I made it themed in my implementation, but I wasn't 100% confident in that decision.
| borderColor: { ...colors, ...borders }, | ||
| fill: { ...colors, ...fills }, | ||
| }, | ||
| shorthands: { |
There was a problem hiding this comment.
Same question here re: shorthand.
| flexDirection: true, | ||
| justifyContent: true, | ||
| alignItems: true, |
There was a problem hiding this comment.
Another thing that's probably worth discussing: do we want flex properties to be part of a Flex component, rather than Box being overloaded?
There was a problem hiding this comment.
I think a Flex and Text component will likely be something we want as a follow-up to rein things in and offer precise dialing. Since Box is meant to be polymorphic by definition we should "allow it all" for quick prototyping and versatility. I imagine Box as a base class for other components/packages (Flex/Text) to extend/inherit from and where prop restriction would happen there.
| fontFamily: vars.font.family, | ||
| fontSize: vars.font.size, | ||
| fontWeight: vars.font.weight, | ||
| lineHeight: vars['line-height'], |
There was a problem hiding this comment.
Same thing here: do we want to expose these properties on the Box component, or would we rather these concerns belong to a Text component?
Summary
First pass at a box component using rainbow-sprinkles. Nice that is allows us to use our
varspackage to map to our tokens. The built CSS is only1.48 kBthanks to dynamic inlining CSS custom properties.Screenshots (if appropriate):