-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[labs] Introduce <Box>and <Flex> components
#7595
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
Conversation
95b2da7 to
88435ea
Compare
Add Box demo and update demo-app componentsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
ericjeney
left a comment
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'll let @mm-wang do the full review, but left a few preliminary comments on the things that jumped out 🙂
|
|
||
| .#{$ns}-box { | ||
| @each $i, $value in $sizes { | ||
| &.gap-#{$i} { |
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.
Should we add #{$ns}- on all of the classes in this file to avoid conflicting with classes that may be defined in consuming apps?
I think that nesting it all under .#{$ns}-box is sufficient to avoid us polluting an application's CSS, but doesn't do anything to prevent the app's CSS from polluting Blueprint's components.
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.
Hm, I'll take the point about app CSS potentially polluting these utlity classes under consideration. 🤔
I'm hesitant to add the namespace to all utility classes mainly because of verbosity. Like, we'd end up with something like this:
<div class="bp6-box bp6-flex bp6-gap-2 bp6-padding-4 bp6-margin-2">That would make DOM inspection pretty cluttered and harder to scan at a glance. On the flip side, the main downside of not adding the prefix is the risk of conflicts with other app-level utility frameworks that could potentially collide with Blueprint's utility classes.
My thinking is that many modern apps already use CSS Modules, CSS-in-JS, or scoped styling approaches, which naturally reduce global namespace pollution. Plus, the .bp6-box parent class already provides some scoping since you have to explicitly apply it to use these utilities. And honestly, users who want comprehensive utility classes are probably already reaching for Tailwind or similar frameworks anyway.
Also worth noting that users primarily interact with <Box> via React props (like <Box gap={2} />), so the actual class names are somewhat of an implementation detail.
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.
We should definitely keep the prefix here, mainly for the reason Eric mentioned, but also for consistency. All of our CSS classes have the prefix, and we shouldn't just change that here. I don't think the additional prefix causes any issues in practice, because like you mentioned people don't write them directly; they either use the Box component, or the Classes constants.
I don't think we should rely on consumers having certain build environments or using certain libraries that may make not having the prefix a non-issue. We can almost guarantee it by just having the namespace, at basically no cost to us.
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.
Prefix added in 6beb43b
Export `FlexProps`Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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 would like to test it out, but wanted to leave these comments here first. I like how this is shaping up though!
I'm curious your thoughts on a Grid layout component as well - I know it doesn't have many properties, but it makes sense to me to have an "alternative" to Flex that has its own set of typical properties.
| /** | ||
| * Slot component. | ||
| */ | ||
| export const Slot = forwardRef<HTMLElement, React.HTMLAttributes<HTMLElement> & { children?: React.ReactNode }>( |
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.
question/nit: As far as I understand, Slot is a placeholder for either Box or Flex type of layout component. I see we're not exposing it by exporting, but I would maybe make it a bit clearer in terms of naming - perhaps LayoutPlaceholder or something to make it clear it's not intended to be a component that will eventually be used. Definitely a nit though! I don't feel super strongly about this.
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.
The primary reason for this being called Slot is that it mirrors/is inspired by the Radix primitive of the same name. I'm open to calling it something else, but I'm not sure if LayoutPlaceholder really conveys the meaning to me more vs. Slot.
| } from "@blueprintjs/core"; | ||
| import { Example, type ExampleProps } from "@blueprintjs/docs-theme"; | ||
|
|
||
| const SPACING_VALUES: Array<BoxProps["padding"]> = [0, 0.5, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10]; |
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.
suggestion: This seems to be used a lot in your examples, should it be an exported constant? I'm not sure - if we add to it/change it in the future, then we run into issues. Would love your thoughts.
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'm personally ok with leaving a bit of duplication (esp. in examples/docs) for now until we find a reason for Blueprint consumers to ask for/need the exported constant. I can't think of a great reason for including it from the inception of these components, but I could be persuaded later / we could always add it later.
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 was convinced that we can add this now. Implementation: cf1d575
| /** | ||
| * The range of values for size as a percentage. | ||
| */ | ||
| type SizeRange = 25 | 50 | 75 | 100; |
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.
question: From the draft, I'd also wondered - why only quarters here? Why not thirds?
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.
Linking to your original comment: #6980 (comment)
Not opposed to adding more options here, but I'm worried that this potentially doesn't scale well to individual classes. How many fractional values do we want to support?
Widths/heights seem like they're perhaps better suited to dynamic computation/inline styles. Something I've toyed around with, but is not formally implemented in this layout components proposal.
Prefix utility classes with `bp6` namespaceBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Revert inclusion of `pt-spacing` CSS variable at rootBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
28c66ce to
784a7cb
Compare
Modify options for align/justify content for closer parity with CSSBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Modify box tests to explicitly test for data attributesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Expose SpacingRange constant and typeBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Revert "Expose SpacingRange constant and type"Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
0a0e02e to
b2ccddd
Compare
Revert "Expose SpacingRange constant and type"Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
This reverts commit cf1d575.
b2ccddd to
78c1ed8
Compare
78c1ed8 to
01345dd
Compare
Move components to labsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
<Box>and <Flex> components<Box>and <Flex> components
| "lint-fix": "es-lint --fix && sass-lint --fix", | ||
| "start": "pnpm dev", | ||
| "test": "vitest run", | ||
| "test:watch": "vitest watch", |
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.
Added this as a convenience helper when writing/running tests locally.
packages/labs/src/common/classes.ts
Outdated
|
|
||
| // declare let BLUEPRINT_NAMESPACE: string | undefined; | ||
| // declare let REACT_APP_BLUEPRINT_NAMESPACE: string | undefined; | ||
| const NS = Classes.getClassNamespace(); |
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.
FYSA @mm-wang, I ended up pulling the namespace from the core package for a few reasons:
- We need to pull in core anyways for the use of the
$pt-spacingvariable that the Box component uses - We need to use the
$nsnamespace in the_box.scssalso, and this was proving to be more complicated with us redeclaring a new namespace in labs - The other packages (
datetime,select, andtable) all follow this same pattern, so there is precedent - We end up with less duplication overall by reusing the namespace already declared in core
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.
We discussed further and decided to combine both approaches. The namespace will now be .bp6-labs-*.
Implemented in e4aa04c
|
|
||
| // Empty for no components right now | ||
| // Example: @use "./sampleComponent/sample-component"; | ||
| /// <reference types="@testing-library/jest-dom/vitest" /> |
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.
The diff here is a bit confusing. A new file, packages/labs/src/vitest-env.d.ts, was added. This was done to pick up the types from @testing-library/jest-dom / fix compilation in the labs package.
Add `-labs` to namespace prefix inside of labs componentsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Add back labs-scoped `DISPLAYNAME_PREFIX`Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
mm-wang
left a comment
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.
LGTM! 🚀
|
|
||
| import { DISPLAYNAME_PREFIX as CORE_DISPLAYNAME_PREFIX } from "@blueprintjs/core"; | ||
|
|
||
| export const DISPLAYNAME_PREFIX = `${CORE_DISPLAYNAME_PREFIX}Labs`; |
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.
➕
Invalidated by push of 12c2cf2
Address docs feedbackBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Add Flex props interface for posterityBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Proposed Changes
Original draft proposal: #6980
This PR introduces new
<Box>,<Flex>, and<Slot>components to@blueprintjs/labs. These components provide flexible layout utilities for margin, padding, gap, alignment, and positioning.Components:
<Box>is the foundational layout primitive with utility props that generate CSS classes<Flex>is a specialized Box component pre-configured withdisplay="flex"<Slot>is used by Box for polymorphic composition viaasChildExamples
Arranging multiple Buttons horizontally:
Or stacked vertically:
Creating more molecular components (such as EntityTitle):