Box: Add design guidelines and update documentation #1358
Conversation
|
Deploy preview for gestalt ready! Built with commit 60e3d7e |
There was a problem hiding this comment.
I can't believe you started with Box 😂 so much work! Nice job!
The layout is a bit confusing…I'm having a hard time telling which header/content goes with which example. I think we need to space things out a bit more.
We're also currently in a "worst of both worlds" with the props table positioning. It's not immediately visible at the top, but it's also not at the bottom, it's…somewhere in the middle. That is going to frustrate a lot of people.
Not sure if this is already planned for future work, but I think we can compress the combination examples a bit more. I get the need for whitespace, but these are displayed pretty inefficiently right now. (But the text doesn't overlap! 😍 )
docs/src/BOILERPLATE.doc.js
Outdated
| 'darkWash', | ||
| ]} | ||
| > | ||
| {(props) => <Box width={60} height={60} rounding="circle" {...props} />} |
There was a problem hiding this comment.
can we avoid spreading props?
We don't allow it (Eslint rule) in Pinboard as a best practice. Codemods can't read them.
|
|
docs/src/Box.doc.js
Outdated
| description={` | ||
| The [media object](http://www.stubbornella.org/content/2010/06/25/the-media-object-saves-hundreds-of-lines-of-code/) is a common pattern for displaying data. What's interesting about this example is the use of \`flex\` to align the items. If you try changing the size of the \`Avatar\` or the number of lines of \`Text\`, both will stay aligned because they are center aligned. | ||
|
|
||
| In the double-sided example we use \`flex="grow"\` to mark a flex child as something that can expand. Try removing the \`grow\` property and see what happens. |
There was a problem hiding this comment.
This is a writer question... what's the tone of these texts?
Try removing the \grow` property and see what happens.` We should discuss the tone of the Docs, is it a friendly voice that guide's you in the discovery of the Docs or just a standard Doc voice that simply describes?
I thnk it's important to get to the voice, maybe in a second round, but the sooner the less we'll have to change work already done.
| <Heading size="sm">{title}</Heading> | ||
| <IconButton | ||
| dangerouslySetSvgPath={{ | ||
| __path: |
There was a problem hiding this comment.
are we reusing this link icon somewhere else? could we create a const and export if so? Maybe it's just used here...
There was a problem hiding this comment.
Will address this separately
|
New comments:
General observations:
|
ayeshakmaz
force-pushed
the
ayesha/update-box-docs
branch
from
14c7491
to
e9c3eb0
Compare
docs/src/Box.doc.js
Outdated
|
|
||
| const cards: Array<Node> = []; | ||
| const card = (c) => cards.push(c); | ||
|
|
||
| card( | ||
| <PageHeader | ||
| name="Box" | ||
| description={`In the darkest night, Box will rise to bring the light. The Lloyd has spoken. | ||
| — Anon _(Winning Box Haiku, 2017)_`} | ||
| description="Box is a component primitive that can be used to build the foundation of pretty much any other component. Using Box allows users to focus on the content, while keeping the pixel details, like spacing, borders, or colors, automatically consistent with the rest of Gestalt." |
There was a problem hiding this comment.
The second sentence is a little awkward (and uses the Oxford comma, which hurts my soul to remove, but I do what Kelly tells me to do 😅). Perhaps something like
"It keeps details like spacing, borders and colors consistent with the rest of Gestalt, while allowing the developer to focus on the content."
docs/src/Box.doc.js
Outdated
| cardSize="md" | ||
| type="do" | ||
| description={` | ||
| Use Box as a building block when creating other components or layouts that do not rely on Flexbox. The included properties should cover any variations needed to create a diverse range of options. |
There was a problem hiding this comment.
nit: "flexbox" shouldn't be capitalized, per MDN: https://developer.mozilla.org/en-US/docs/Learn/CSS/CSS_layout/Flexbox#introducing_a_simple_example
docs/src/Box.doc.js
Outdated
| <MainSection.Card | ||
| cardSize="md" | ||
| type="don't" | ||
| description={`Avoid using arbitrary \`<div>\` elements. Instead, when building a component, prioritize using Box. If you need to set a custom style, you can do so using the \`dangerouslySetInlineStyle\` prop. However, this should be avoided whenever possible by utilizing the other props provided in Box.`} |
There was a problem hiding this comment.
You may also want to mention / link to our lint rule for this
| <MainSection.Card | ||
| cardSize="lg" | ||
| defaultCode={` | ||
| function BoxFlyoutExample() { |
There was a problem hiding this comment.
it would still be nice to be able to close the Flyout 😅
docs/src/Box.doc.js
Outdated
| function Example() { | ||
| const HEADER_ZINDEX = new FixedZIndex(100); | ||
| const zIndex = new CompositeZIndex([HEADER_ZINDEX]); | ||
| return <Box color="blue" width={60} height={60} zIndex={zIndex} /> | ||
| return <Box color="midnight" width={60} height={60} zIndex={zIndex} /> |
There was a problem hiding this comment.
I think this would be better demonstrated with two (or more) Boxes with various z-indices? A single Box doesn't convey the layering aspect
|
|
||
| card( | ||
| <MainSection | ||
| name="Related" |
There was a problem hiding this comment.
I agree in general but think it's fine to break that rule for Box, since it's so foundational. I don't think it's really clear when a user would want Card, Column, or Letterbox
(this can def be addressed in a future PR)
| </Box> | ||
| )} | ||
|
|
||
| <Box width="80%" marginTop={-2} marginBottom={6}> |
There was a problem hiding this comment.
Do we want to render this empty Box if there is no description? I would think "no" in general
There was a problem hiding this comment.
We actually do in this case to account for a funky spacing thing
There was a problem hiding this comment.
That doesn't sound like the right solution. Can that be solved by using flex properties on the parent/siblings?
There was a problem hiding this comment.
Wrapped it in a Box instead since Flex doesn't have margin
Update Box docs to new style
Test Plan
Lots of manual testing and feedback gathering