Add docs for {Flex,FlexItem}Component #190
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/view-components/j8muzbei8 |
|
I feel like our current layout for PVC is not working well for something complex like flex. Comparing https://primer.style/css/utilities/flexbox#align-items to my WIP here on https://view-components-git-fork-srt32-st-docs-for-flex-item.primer.vercel.app/view-components/components/flex, I am not sure this page is going to scale nicely... |
srt32
changed the title
| # | ||
| # @param justify_content [Symbol] Use this param to distribute space between and around flex items along the main axis of the container. <%= one_of(Primer::FlexComponent::JUSTIFY_CONTENT_OPTIONS) %> | ||
| # @param inline [Boolean] Defaults to false. | ||
| # @param flex_wrap [Boolean] Defaults to nil. |
There was a problem hiding this comment.
This is a boolean but also defaults to nil... what's the right thing here?
There was a problem hiding this comment.
@srt32 that's a good question. What do we do in Primer React?
There was a problem hiding this comment.
Good question!
I also just noticed this on https://primer.style/components/Flex
Previously, a Flex.Item component was used for flex item specific properties; Box now contains all those properties and should be used in its place.
Maybe we should deprecate FlexItem?
I am having a hard time grokking the PRC (I'm new to the codebase) but it looks to mainly delegate to Box, which delegates to styleSystem.
The API though is directly
<Flex flexWrap="nowrap">
There was a problem hiding this comment.
@emplums what do you think? I'd generally lean towards deprecating all of the Flex-related components and just encourage the usage of Box with the flex-related system arguments.
There was a problem hiding this comment.
@joelhawksley the proposed game plan for PRC is that Box and Flex will have all the same properties, the only difference is Flex will have display: flex set by default. The idea is that it will be handy shortcut from having to type display: flex over and over, and will still keep the ease of grokking the code to see exactly where flex is being used
|
This PR is ready for another look. I'd love to get this merged so we can call everything existing documented as we sort out where we're going with Flex. |
| # <div class="p-5 border bg-gray-light">Item 1</div> | ||
| # <div class="p-5 border bg-gray-light">Item 2</div> | ||
| # <div class="p-5 border bg-gray-light">Item 3</div> | ||
| # <%= render(Primer::BoxComponent.new(p: 5, classes: "border bg-gray-light")) { "Item 1" } %> |
There was a problem hiding this comment.
Interesting. Are there not system arguments for the border/bg classes?
There was a problem hiding this comment.
Attempting to not get bogged down here...
<%= render(Primer::BoxComponent.new(p: 5, bg: :gray_light, border: true)) { "Item 1" } %>
which is what I assumed would work, did not for border (I will update for bg!):
which we think is because of
view_components/lib/primer/classify.rb
Lines 216 to 217 in 638d97c
There was a problem hiding this comment.
Oh! Looks like we need to add support for border: true somehow. Mind filing a separate issue?
| flex_wrap false | ||
| end | ||
|
|
||
| content do |
There was a problem hiding this comment.
@manuelpuyol is there anything you think we could do to avoid the html_safe here?
This change adds docs for
FlexComponentandFlexItemComponent. There is a lot we could show here and I went with a handful of examples that exercises most of the knobs but not all the possible options.