Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

docs: layout guide #1091

Merged
merged 8 commits into from
Mar 25, 2019
Merged

docs: layout guide #1091

merged 8 commits into from
Mar 25, 2019

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented Mar 21, 2019

Introduces first version of Stardust Layout guide.

This version is specifically focused on

  • delivering principles that should guide towards selecting one layout component over another
  • providing key "dont's" to prevent improper use of Stardust components

Apart from that, several style issues in Stardust guides were addressed.

@codecov
Copy link

codecov bot commented Mar 21, 2019

Codecov Report

Merging #1091 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1091   +/-   ##
=======================================
  Coverage   82.09%   82.09%           
=======================================
  Files         711      711           
  Lines        8528     8528           
  Branches     1224     1224           
=======================================
  Hits         7001     7001           
  Misses       1511     1511           
  Partials       16       16
Impacted Files Coverage Δ
packages/react/src/components/Box/Box.tsx 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b8bc3d...cf75aae. Read the comment docs.

{code('Grid')}.
</li>
</ul>
<Header as="h3">Note on Segment component</Header>
Copy link
Member

Choose a reason for hiding this comment

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

Actually, we should have the same note about Box component

Copy link
Member

Choose a reason for hiding this comment

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

nit: We can move this section after Flex recipes

Copy link
Contributor Author

@kuzhelov kuzhelov Mar 24, 2019

Choose a reason for hiding this comment

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

Notes on Box component

speaking of the Box component - it seems that the first thing we should do is rather fix its description. I've intentionally consulted about whether the Box component is seen now as a layout one, given the recent change of its description: https://github.com/stardust-ui/react/blob/master/packages/react/src/components/Box/Box.tsx#L20

Thus, lets just make things consistent - change the description of Box component and put an update to the guides then - otherwise it will just create conflicting thoughts in the client's head.

Why not put after Flex recipies

This is, actually, done intentionally. Given current usage statistics, the problems that are described in this 'Notes' section is much more serious (especially taking their conequences into account) rather than the ones introduced by overuse of, say, Flex.Item. Thus, it is very important to make this note earlier than later.

Another point relates to where this paragraph semantically attributes to: it definitely should be a part of the section where Stardust layout components are enumerated, with the corresponding use-cases for each - and thus I would rather leave it here and won't introduce a separate section for it.

Copy link
Member

Choose a reason for hiding this comment

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

We added note about Segment, it's cool 👍

But, I want avoid the following thing: "Okay, I can't use Segment... Oh, we have have Box, let's do <Box styles={{ display: 'flex' }} />!" I.e. Box can be abused for layouts purposes, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree - then, if we agree on fixing Box description (which now states that it is a layout component), then let me do that and introduce a dedicated section to the Layout guide

<Flex.Item>
<Button content="Deny" />
</Flex.Item>
</Flex>
Copy link
Member

Choose a reason for hiding this comment

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

I like the way how it's done on overreacted.io:
image


image

Something like this will be really nice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, fully agree - I would say, anything that would allow us to have comments preserved would be already a huge win :) for now can do either of two:

  • use the approach currently taken
  • or write code twice - one for example's code, one for the actual rendering

Given these options, have chosen the first approach because, essentially, it will at least guarantee that these two things (code and rendered output) won't go out of sync

Copy link
Member

Choose a reason for hiding this comment

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

Let's leave as is for now, duplication is more evil 😈

@hughreeling
Copy link
Contributor

Could we add linked to the layout guide in the Flex, Segment, Box, Layout components? (Put in description as link?)

Let's also update description of Box and Segment and explicitly say what they're used for and not used for.

Having this information duplicated isn't a bad thing....

@kuzhelov kuzhelov merged commit 9c30100 into master Mar 25, 2019
@delete-merged-branch delete-merged-branch bot deleted the docs/add-layout-guide branch March 25, 2019 13:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants