Skip to content

Conversation

elileto
Copy link
Contributor

@elileto elileto commented Dec 17, 2018

WHY are these changes introduced?

Resolves #536

Stack Item properties needed to be listed in the documentation.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

🎩 checklist

@elileto elileto self-assigned this Dec 17, 2018
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 19:33 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from cf2d77c to f0f792d Compare December 17, 2018 19:48
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 19:48 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from f0f792d to a8264e3 Compare December 17, 2018 20:23
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 20:23 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 20:58 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from ad54b94 to 48fe810 Compare December 17, 2018 21:01
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 21:01 Inactive

| Prop | Type | Description | Default |
| -------- | ------- | -------------------------------------------------------------------------- | ------- |
| fill | boolean | Adjust vertical alignment of elements to the right edge of another element | false |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| fill | boolean | Adjust vertical alignment of elements to the right edge of another element | false |
| fill | boolean | Fill the available horizontal space in the stack with the item | false |


## Stack item

Use item to align buttons or secondary content to the right edge of another element, allowing it to wrap below on small screens.
Copy link
Member

Choose a reason for hiding this comment

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

The Stack automatically wraps each of its children in a Stack.Item (unless the implementor has already wrapped a child/children with Stack.Item). It may be helpful to mention that, and then explain that you can use the subcomponent directly with the fill prop when you want the item to fill the available horizontal space in the stack (and mention they can see this in action in the "Stack where a single item fills the remaining space" example above).

@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 48fe810 to 21d8c8b Compare December 17, 2018 22:08
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 17, 2018 22:08 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 21d8c8b to 750cd42 Compare December 18, 2018 15:39
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 18, 2018 15:39 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 750cd42 to 30656e0 Compare December 18, 2018 15:43
@BPScott BPScott temporarily deployed to polaris-react-pr-772 December 18, 2018 15:43 Inactive
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Looks good! But we can't 🎩 it yet :(


## Stack item

The Stack will implicitly wrap each of its children in a Stack.Item unless used directly. Use Stack.Item as a subcomponent with the fill prop to have the item fill the available horizontal space in the stack. See "Stack where a single item fills the remaining space" as an example of this above.
Copy link
Member

Choose a reason for hiding this comment

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

The Stack will implicitly wrap each of its children in a Stack.Item unless used directly 👍 I'm glad we've finally gotten this documented ❤️

Choose a reason for hiding this comment

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

Yes! I'm also happy to see this get documented!

I don't think we use determiners before component names, unless it's followed by the word "component". So, something like The stack component will implicitly....

In documentation, we also use sentence case for components, ie: a stack item vs. Stack.Item. https://docs.google.com/document/d/1zVDsHIWhoir2svRjdtSdRbD_ruTz3K1nAJcQLGPVQwM/edit#heading=h.lm3ja0wzryt7

It might be a good idea to link to the example using a named anchor. The resource list documentation has an example of this.


| Prop | Type | Description | Default |
| -------- | ------- | ------------------------------------------------------------- | ------- |
| fill | boolean | Fil the available horizontal space in the stack with the item | false |
Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth it to add these to the interface as well?

@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 30656e0 to e8fb11a Compare January 7, 2019 15:11
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 7, 2019 15:12 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from e8fb11a to 1b43136 Compare January 7, 2019 15:20
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 7, 2019 15:20 Inactive
@elileto elileto requested a review from AndrewMusgrave January 7, 2019 15:22
@elileto elileto changed the title [WIP] [Stack.Item] Add Stack.Item properties to style guide documentation [Stack.Item] Add Stack.Item properties to style guide documentation Jan 7, 2019
Copy link

@solonaarmstrong-zz solonaarmstrong-zz left a comment

Choose a reason for hiding this comment

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

This is great! Just a couple comments.

@elileto elileto requested a review from chloerice January 11, 2019 14:46
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 1975dcc to 2066e36 Compare January 16, 2019 14:05
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 16, 2019 14:05 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 2066e36 to 685f1c3 Compare January 18, 2019 19:25
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 18, 2019 19:25 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 685f1c3 to 36cada3 Compare January 18, 2019 19:26
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 18, 2019 19:26 Inactive

## Stack item

The stack component will wrap each item in a stack item component regardless if the item has already been deliberately wrapped in a stack item component or not. However, When wrapping the stack component with a stack item deliberately, use the fill prop to force the item to fill the remaining horizontal space in the stack. See the [Stack where a single item fills the remaining space](#single-item-fills-remaining-space) example.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @selenehinkley I updated this again like we discussed! Mind taking another look? 🙂

Copy link

@solonaarmstrong-zz solonaarmstrong-zz Jan 19, 2019

Choose a reason for hiding this comment

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

The stack component will wrap each item in a stack item component regardless if the item has already been deliberately wrapped in a stack item component or not.

To me that sounds like an item could potentially be wrapped twice in a stack item component. The word "regardless" sounds like it ignores what it's been given and goes ahead and wraps another stack item around it.

When what it actually does is wrap each item in a stack item component if it hasn't already been deliberately wrapped.


However, When wrapping the stack component with a stack item deliberately, use the fill prop...

This sounds like it's mandatory. To me, this reads like "if you decide to wrap it deliberately, you must use the fill prop". I think it's the word "however".

(Also, the W needs to be lowercase).


One more thing... would it make sense to mention why else a stack item would be used? One other use case is if a consumer wants to wrap multiple elements so they're treated like one item.

import * as styles from '../../Stack.scss';

export interface Props {
/** Elements to display inside stack */

Choose a reason for hiding this comment

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

Suggested change
/** Elements to display inside stack */
/** Elements to display inside the item */

@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 36cada3 to 99301e3 Compare January 21, 2019 14:52
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 21, 2019 14:52 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 99301e3 to cfe4949 Compare January 21, 2019 15:33
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 21, 2019 15:33 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from cfe4949 to 7c869c2 Compare January 21, 2019 15:42
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 21, 2019 15:42 Inactive
@elileto elileto force-pushed the add-Stack-Item-properties-to-documentation branch from 7c869c2 to 3eec4ea Compare January 21, 2019 20:58
@BPScott BPScott temporarily deployed to polaris-react-pr-772 January 21, 2019 20:59 Inactive
@elileto
Copy link
Contributor Author

elileto commented Jan 21, 2019

Testing is failing for the DropZone component. Will look into this first thing Wednesday.


## Stack item

The stack component will treat multiple elements wrapped in a stack item component as one item. By default, each individual element is treated as one stack item. Use the fill prop on a single stack item component to make it fill the rest of the available horizontal space. See the [Stack where a single item fills the remaining space](#single-item-fills-remaining-space) example.

Choose a reason for hiding this comment

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

Perfect!! ❤️

@solonaarmstrong-zz
Copy link

Drop zone shouldn't be affected by this PR and it looks like all the checks are passing. 🎉

@elileto elileto merged commit 36958d8 into master Jan 21, 2019
@solonaarmstrong-zz solonaarmstrong-zz deleted the add-Stack-Item-properties-to-documentation branch January 22, 2019 01:48
@BPScott BPScott temporarily deployed to production January 28, 2019 16:33 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants