1885 bug splitpagelayoutpane needs to be able to consume heading 1

[agreenberry]

Describe your changes here.

Closes #1885

Screenshots

Please provide before/after screenshots for any visual changes
(View in Storybook)

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7c2cf79

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

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

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	104.2 KB (-0.01% 🔽)
dist/browser.umd.js	104.74 KB (-0.01% 🔽)

[andrialexandrou]

Requesting some changes about component interface and tests.

I have another question that betrays my ignorance: does this repo contain documentation for these components as well? I'd like to review the documentation to make sure it adequately communicates how to include headings with this component.

[andrialexandrou]

The component interface should support accepting a heading level, and in my opinion should make it a required argument.

In this case I believe we were wanting the heading to be a level 2, for instance, but I could imagine circumstances it might be a level 1 or a level 3.

I can see by the tests that it is a level 2, but I'm thinking here in terms of component design.

[andrialexandrou]

From what I can tell this is still choosing h2 by default, and I propose we require user to provide heading level.

[andrialexandrou]

I'd like to review the documentation to make sure it adequately communicates how to include headings with this component.

Ah, the answer is yes. Let me propose the change...

It looks like we need to add an item for the heading here as well? IIUC

react/src/SplitPageLayout/SplitPageLayout.docs.json

Lines 69 to 130 in a19b721

	"name": "SplitPageLayout.Pane",
	"props": [
	{
	"name": "position",
	"type": "| 'start' | 'end' | { narrow?: | 'start' | 'end' regular?: | 'start' | 'end' wide?: | 'start' | 'end' }",
	"defaultValue": "'start'",
	"description": ""
	},
	{
	"name": "width",
	"type": "| 'small' | 'medium' | 'large'",
	"defaultValue": "'medium'",
	"description": "The width of the pane."
	},
	{
	"name": "resizable",
	"type": "boolean",
	"defaultValue": "false",
	"description": "When true, the pane may be resized by the user."
	},
	{
	"name": "widthStorageKey",
	"type": "string",
	"defaultValue": "'paneWidth'",
	"description": "Provide a key used by localStorage to persist the size of the pane on the client."
	},
	{
	"name": "sticky",
	"type": "boolean",
	"defaultValue": "true",
	"description": "Whether the pane should stick to the top of the screen while the content scrolls."
	},
	{
	"name": "offsetHeader",
	"type": "number | string",
	"defaultValue": "0",
	"description": "Use offsetHeader along with the sticky prop to push the sticky pane down to make room for a sticky header if necessary. Use the type `string` to specify the height with a unit i.e. 5rem; otherwise the type `number` will be taken as px."
	},
	{
	"name": "padding",
	"type": "| 'none' | 'condensed' | 'normal'",
	"defaultValue": "'normal'",
	"description": "The amount of padding inside the pane."
	},
	{
	"name": "divider",
	"type": "| 'none' | 'line' | { narrow?: | 'none' | 'line' | 'filled' regular?: | 'none' | 'line' wide?: | 'none' | 'line' }",
	"defaultValue": "'line'",
	"description": ""
	},
	{
	"name": "hidden",
	"type": "| boolean | { narrow?: boolean regular?: boolean wide?: boolean }",
	"defaultValue": "false",
	"description": "Whether the pane is hidden."
	},
	{
	"name": "sx",
	"type": "SystemStyleObject"
	}
	]
	},

[andrialexandrou]

github/accessibility-audits#3506

[kendallgassner]

Curious if it makes more sense to make the heading a component itself rather then passing it in as a prop:

          <SplitPageLayout.Pane.heading as='h3'>Custom heading"</SplitPageLayout.Pane.heading >

^ this would make it easier for people to add attributes in the future.

[TylerJDev]

@andrialexandrou, @kendallgassner - I would love to get y'alls opinion on the additions to this PR. I think I covered all of the comments left on this PR, but want to make sure I've captured all of the criteria needed to close the main issue out.

[kendallgassner]

Suggested change

import Heading from '../Heading'

[kendallgassner]

small nit to remove an unused dependency but other-wise this looks great!