SplitPageLayout API

[colebemis]

Summary

This PR includes documentation for the proposed API of the SplitPageLayout component. I'm looking for feedback on the API before I begin implementation.

Here's a preview of the SplitPageLayout documentation: https://primer-48c11ddef3-13348165.drafts.github.io/SplitPageLayout

Here's a diagram of the proposed API:

(FigJam file)

Note about responsive props

SplitPageLayout adopts the responsive prop API that @vdepizzol proposed in this ADR.

Any prop marked "responsive" will optionally accept an object mapping viewport ranges to prop values. Example:

// Use "line" divider for all viewport ranges
<SplitPageLayout.Header divider="line">

// Use "line" divider on regular viewports and "filled" for narrow viewports
<SplitPageLayout.Header dividier={{regular: 'line', narrow: 'filled'}}>

Part of https://github.com/github/primer/issues/793

[changeset-bot]

⚠️ No Changeset found

Latest commit: 6e60bdf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	67.6 KB (0%)
dist/browser.umd.js	67.97 KB (0%)

[siddharthkp]

yay! makes so much sense

[siddharthkp]

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

[colebemis]

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

[jonrohan]

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.

[colebemis]

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.

[vdepizzol]

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

[vdepizzol]

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

[alliethu]

@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.

[owenniblock]

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.

[colebemis]

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.

[joelhawksley]

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.

[mperrotti]

+1 for "hidden". For hidden FormControl labels, we use a prop named visuallyHidden.

[mperrotti]

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

[colebemis]

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.

[mperrotti]

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

[colebemis]

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

[vdepizzol]

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.

[colebemis]

I'm going to go ahead and merge this draft to keep SplitPageLayout moving forward. Feel free to leave any additional feedback about the SplitPageLayout or PageLayout API in the #primer-react channel in Slack.