Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SplitPageLayout API #2144

Merged
merged 7 commits into from
Jun 30, 2022
Merged

SplitPageLayout API #2144

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6117622
Draft SplitPageLayout API docs
colebemis Jun 23, 2022
c5c53a5
Merge branch 'main' into splitpagelayout-api
colebemis Jun 24, 2022
a9c7353
Update docs/content/SplitPageLayout.mdx
colebemis Jun 24, 2022
96edc7a
Update docs/content/SplitPageLayout.mdx
colebemis Jun 24, 2022
64d76df
visible -> hidden prop
colebemis Jun 30, 2022
2578d2e
Merge branch 'splitpagelayout-api' of github.com:primer/react into sp…
colebemis Jun 30, 2022
6e60bdf
Merge branch 'main' into splitpagelayout-api
colebemis Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
313 changes: 313 additions & 0 deletions docs/content/SplitPageLayout.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,313 @@
---
title: SplitPageLayout
componentId: split_page_layout
status: Draft
# description: TODO
colebemis marked this conversation as resolved.
Show resolved Hide resolved
---

<Note variant="warning">Not implemented yet</Note>

## Examples

### Default

```jsx
<SplitPageLayout>
<SplitPageLayout.Header>
<Placeholder label="Header" height={64} />
colebemis marked this conversation as resolved.
Show resolved Hide resolved
</SplitPageLayout.Header>
<SplitPageLayout.Pane>
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer>
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
colebemis marked this conversation as resolved.
Show resolved Hide resolved
```

### With pane hidden on narrow viewports

```jsx
<SplitPageLayout>
<SplitPageLayout.Header>
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Pane visible={{narrow: false}}>
Copy link
Member

@siddharthkp siddharthkp Jun 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

nit: I generally prefer optional boolean attributes to be true instead of false, especially when the default is visible=true: <SplitPageLayout.Pane hidden={{narrow: true}}>

"visible: no, not visible" vs "hidden: yes, hidden"

bonus points for matching/extending the HTML attribute hidden https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/hidden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like hidden 👍 (especially because it aligns with an existing HTML attribute). @vdepizzol @jonrohan how do you feel about using hidden instead of visible?

Copy link
Member

Choose a reason for hiding this comment

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

I think there was push back because it matched html spec. In the PVC case hidden: true would end up being attributes on the html tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason we don't want the attribute to end up on the HTML tag? Seems like it would be semantically correct:

The hidden global attribute is a Boolean attribute indicating that the element is not yet, or is no longer, relevant.The hidden global attribute is a Boolean attribute indicating that the element is not yet, or is no longer, relevant.

Copy link
Contributor

@vdepizzol vdepizzol Jun 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I also prefer hidden, but like @jonrohan mentioned, there's a big confusion in the way PVC is designed, since it handles element attributes and component properties the same way.

Through system_arguments, a boolean hidden HTML attribute can be passed to any PVC already. Since the visibility prop also needs to accept responsive values (narrow/regular/wide), and doesn't manipulate the hidden attribute directly, we tried to find an alternative.

If you folks think this is not a problem, I'm happy to use hidden instead. I'd just like to make sure the API is the same across PRC and PVC.

@jonrohan what do you think about dropping support for system_arguments in PageLayout and SplitPageLayout? We definitely don't want any of those utilities applied, and I can't think of any custom HTML attribute being useful there (maybe data-*?)

c/c @joelhawksley

Copy link
Contributor

Choose a reason for hiding this comment

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

in general I'd like system_argument support to be turned off by default, I think moving that way for any new components is the way to go

@jonrohan yes please <3

Copy link

Choose a reason for hiding this comment

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

@vdepizzol I'm not familiar with the hidden property as I've only ever used/recommended display: none. I'm cc'ing @primer/accessibility-reviewers on this one.

Copy link
Contributor

@owenniblock owenniblock Jun 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I'm fully grokking what you're asking here so apologies in advance if I don't answer your question correctly 😅

To hide an element visually but still allow the screen reader to access it, we style it so that it appears off the page (e.g. <h1 class="sr-only">A heading which a screen reader will read</h1> will achieve this).

However, if you try to apply this on any content which contains focusable elements (buttons, and other form elements) this causes confusion for sighted users who navigate via keyboard since the elements will gain keyboard focus but the user won't be able to see the element that's focused.

In the case of panels that could contain focusable elements, you'd have to hide this from all users to avoid confusion since it's most likely that there will be focusable elements within the panel. I would advise using display: none on a css class for this since that seems to be the pattern we use and recommend elsewhere.

As a slight aside, I would also recommend when you're building a prototype for this component that it's tested early with a high zoom to ensure that developers who use high zoom don't find panels removed unnecessarily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank y'all for weighing in. Based on everyone's input so far, I'm going to move forward with the hidden prop for this API draft. It seems like we agree that calling the prop hidden is better than visible from a component API perspective, but we still need to figure out the implementation details. We can discuss those details in future PRs.

Copy link

Choose a reason for hiding this comment

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

Chiming in a bit late here, but I'd like to consider using something besides hidden. I think we need to be careful of overloading terms in our glossary, and hidden is also an HTML attribute.

While I do generally agree that having system arguments coexist with HTML attributes, in this case I think it's actually a good thing! IMO we should be leaving room for HTML to supercede our component systems as it it ultimately the underlying technology we're building on.

<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer>
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
```

### With content hidden on narrow viewports

```jsx
<SplitPageLayout>
<SplitPageLayout.Header>
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Pane>
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content visible={{narrow: false}}>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer>
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
```

### Without dividers

```jsx
<SplitPageLayout>
<SplitPageLayout.Header divider="none">
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Pane divider="none">
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer divider="none">
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
colebemis marked this conversation as resolved.
Show resolved Hide resolved
```

### With pane on right

```jsx
<SplitPageLayout>
<SplitPageLayout.Header>
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Pane position="end">
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Footer>
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
```

### Without padding

```jsx
<SplitPageLayout>
<SplitPageLayout.Header padding="none">
colebemis marked this conversation as resolved.
Show resolved Hide resolved
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Pane padding="none">
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content padding="none">
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer padding="none">
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
```

### Without header or footer

```jsx
<SplitPageLayout>
<SplitPageLayout.Pane>
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
</SplitPageLayout>
Copy link
Member

Choose a reason for hiding this comment

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

yay! makes so much sense

```

### With non-sticky pane

```jsx
<SplitPageLayout>
<SplitPageLayout.Header>
<Placeholder label="Header" height={64} />
</SplitPageLayout.Header>
<SplitPageLayout.Pane sticky={false}>
colebemis marked this conversation as resolved.
Show resolved Hide resolved
colebemis marked this conversation as resolved.
Show resolved Hide resolved
<Placeholder label="Pane" height={120} />
</SplitPageLayout.Pane>
<SplitPageLayout.Content>
<Placeholder label="Content" height={240} />
</SplitPageLayout.Content>
<SplitPageLayout.Footer>
<Placeholder label="Footer" height={64} />
</SplitPageLayout.Footer>
</SplitPageLayout>
```

## Props

### SplitPageLayout

<PropsTable>
<PropsTableSxRow />
</PropsTable>

### SplitPageLayout.Header

<PropsTable>
<PropsTableRow
name="divider"
// TODO: Document ResponsiveValue
// This prop accepts a viewport range map (i.e. {regular: ..., narrow: ...}) in addition to a single value.
// The `regular` and `wide` keys of the viewport range map can be 'none' or 'line'.
// The `narrow` key of the viewport range map can be 'none' or 'line' or 'filled'.
type={`ResponsiveValue<
Copy link
Contributor

Choose a reason for hiding this comment

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

ResponsiveValue<> is a little cryptic. Should we explicitly write out the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. ResponsiveValue<> doesn't explain much. The full type signature looks like this:

'none' | 'line' | { narrow?: 'none' | 'line' | 'filled'; regular?: 'none' | 'line'; wide?: 'none' | 'line' }

Do you think it'd be overwhelming to show the whole thing?

My original plan was to add a separate documentation page about responsive props that explains what ResponsiveValue<> means and link to it from here.

'none' | 'line',
'none' | 'line' | 'filled'
>`}
colebemis marked this conversation as resolved.
Show resolved Hide resolved
defaultValue="'line'"
/>
<PropsTableRow
name="visible"
type={`ResponsiveValue<boolean>`}
defaultValue="true"
description="Whether the header is visible."
/>
<PropsTableRow
name="padding"
type={`| 'none'
| 'normal'
| 'condensed'`}
defaultValue="'normal'"
/>
<PropsTableSxRow />
</PropsTable>

### SplitPageLayout.Content

<PropsTable>
<PropsTableRow
name="width"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be called "maxWidth" if it's the maximum width of the content area?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. We'd need to update this in PageLayout as well. @vdepizzol @jonrohan How you feel about width -> maxWidth?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should remain as width. As a prop, it's a characteristic of the region, not a reference to the CSS attribute. Responsive components are expected to adjust on smaller viewport sizes anyway.

type={`| 'full'
| 'medium'
| 'large'
| 'xlarge'`}
defaultValue="'xlarge'"
description="The maximum width of the content region."
/>
<PropsTableRow
name="visible"
type={`ResponsiveValue<boolean>`}
defaultValue="true"
description="Whether the content is visible."
/>
<PropsTableRow
name="padding"
type={`| 'none'
| 'normal'
| 'condensed'`}
defaultValue="'normal'"
/>
<PropsTableSxRow />
</PropsTable>

### SplitPageLayout.Pane

<PropsTable>
<PropsTableRow
name="position"
type={`ResponsiveValue<
'start' | 'end'
>`}
defaultValue="'start'"
/>
<PropsTableRow
name="width"
type={`| 'small'
| 'medium'
| 'large'`}
defaultValue="'medium'"
description="The width of the pane."
/>
<PropsTableRow
name="divider"
type={`ResponsiveValue<
'none' | 'line',
'none' | 'line' | 'filled'
>`}
defaultValue="'line'"
/>
<PropsTableRow
name="visible"
type={`ResponsiveValue<boolean>`}
defaultValue="true"
description="Whether the pane is visible."
/>
<PropsTableRow
name="padding"
type={`| 'none'
| 'normal'
| 'condensed'`}
defaultValue="'normal'"
/>
<PropsTableSxRow />
</PropsTable>

### SplitPageLayout.Footer

<PropsTable>
<PropsTableRow
name="divider"
type={`ResponsiveValue<
'none' | 'line',
'none' | 'line' | 'filled'
>`}
defaultValue="'line'"
/>
<PropsTableRow
name="visible"
type={`ResponsiveValue<boolean>`}
defaultValue="true"
description="Whether the footer is visible."
/>
<PropsTableRow
name="padding"
type={`| 'none'
| 'normal'
| 'condensed'`}
defaultValue="'normal'"
/>
<PropsTableSxRow />
</PropsTable>

## Status

<ComponentChecklist
items={{
propsDocumented: true,
noUnnecessaryDeps: false,
adaptsToThemes: false,
adaptsToScreenSizes: false,
fullTestCoverage: false,
usedInProduction: false,
usageExamplesDocumented: false,
hasStorybookStories: false,
designReviewed: false,
a11yReviewed: false,
stableApi: false,
addressedApiFeedback: false,
hasDesignGuidelines: false,
hasFigmaComponent: false
}}
/>