-
Notifications
You must be signed in to change notification settings - Fork 648
1885 bug splitpagelayoutpane needs to be able to consume heading 1 #3143
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.
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
Changes from 10 commits
4609667
68377a2
ac0e806
86db7cc
830f952
297b9fe
051719c
a05f5cc
6047711
99431fa
5cbaeb3
f963fe7
6b0123c
2151877
45b59f1
eb26f54
8dabe2f
f87d940
8dbe325
cda5fc7
7c2cf79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@primer/react': minor | ||
| --- | ||
|
|
||
| Added Heading to SplitPageLayout.Pane |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||
| import React from 'react' | ||||
| import {createGlobalStyle} from 'styled-components' | ||||
| import Box from '../Box' | ||||
| import Heading from '../Heading' | ||||
|
||||
| import Heading from '../Heading' |
TylerJDev marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just expose this as a subComponent:
PageLayout.Pane.Heading = Heading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted this in my most recent commit. There's now a new slot for a heading component that's tied to PageLayout. This should allow someone to use PageLayout.PaneHeading to place a heading component within the pane, above everything else (pending feedback).
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,4 +44,30 @@ describe('SplitPageLayout', () => { | |
|
|
||
| expect(pane.getAttribute('id')).toBe('customId') | ||
| }) | ||
|
|
||
| it('renders Pane with a heading', () => { | ||
| const {getByText} = render( | ||
| <ThemeProvider> | ||
| <SplitPageLayout> | ||
| <SplitPageLayout.Pane heading="Custom heading" /> | ||
|
||
| </SplitPageLayout> | ||
| </ThemeProvider>, | ||
| ) | ||
| const heading = getByText('Custom heading') | ||
|
|
||
| expect(heading.tagName).toBe('H2') | ||
agreenberry marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| }) | ||
|
|
||
| it('renders Pane with a custom level heading', () => { | ||
| const {getByText} = render( | ||
| <ThemeProvider> | ||
| <SplitPageLayout> | ||
| <SplitPageLayout.Pane heading="Custom heading" headingLevel="h3" /> | ||
| </SplitPageLayout> | ||
| </ThemeProvider>, | ||
| ) | ||
| const heading = getByText('Custom heading') | ||
|
|
||
| expect(heading.tagName).toBe('H3') | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell this is still choosing
h2by default, and I propose we require user to provide heading level.