-
Notifications
You must be signed in to change notification settings - Fork 638
Revert "Remove Box usage and sx
prop from PageLayout (#6872)"
#6940
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
Conversation
This reverts commit 4e797ef.
|
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.
Pull Request Overview
This PR reverts a previous commit that removed Box usage and the sx
prop from PageLayout components. The revert is necessary because the original changes conflict with the useSlots strategy, and the team plans to fix the implementation in a future release.
- Restores the
sx
prop to all PageLayout components and subcomponents - Re-adds Box/BoxWithFallback usage throughout the PageLayout implementation
- Reverts CSS module changes that were part of the original removal
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/styled-react/src/index.tsx | Restores PageLayout export from @primer/react instead of local implementation |
packages/styled-react/src/components/PageLayout.tsx | Removes the entire styled-components wrapper implementation |
packages/react/src/PageLayout/PageLayout.tsx | Re-adds sx prop support and BoxWithFallback usage across all components |
packages/react/src/PageLayout/PageLayout.stories.tsx | Adds sx prop to story template |
packages/react/src/PageLayout/PageLayout.module.css | Removes draggable handle CSS classes |
packages/react/src/PageLayout/PageLayout.dev.stories.tsx | Converts CSS classes to sx prop usage |
packages/react/src/PageLayout/PageLayout.dev.stories.module.css | Removes CSS file entirely |
.changeset/gold-geckos-send.md | Removes changeset for the original breaking change |
<Box | ||
sx={{ | ||
position: 'absolute', | ||
inset: '0 -2px', | ||
cursor: 'col-resize', | ||
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'transparent', | ||
transitionDelay: '0.1s', | ||
'&:hover': { | ||
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'neutral.muted', | ||
}, |
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 empty fragment wrapper <>...</>
around the Box component is unnecessary. The Box can be returned directly without the fragment wrapper.
See below for a potential fix:
<Box
sx={{
position: 'absolute',
inset: '0 -2px',
cursor: 'col-resize',
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'transparent',
transitionDelay: '0.1s',
'&:hover': {
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'neutral.muted',
},
}}
role="slider"
aria-label="Draggable pane splitter"
aria-valuemin={minWidth}
aria-valuemax={maxWidth}
aria-valuenow={currentWidth}
aria-valuetext={`Pane width ${currentWidth} pixels`}
tabIndex={0}
onMouseDown={event => {
if (event.button === 0) {
setIsDragging(true)
onDragStart?.()
}
}}
onKeyDown={event => {
if (
event.key === 'ArrowLeft' ||
event.key === 'ArrowRight' ||
event.key === 'ArrowUp' ||
event.key === 'ArrowDown'
) {
setIsKeyboardDrag(true)
onDragStart?.()
}
}}
onDoubleClick={onDoubleClick}
/>
Copilot uses AI. Check for mistakes.
}} | ||
onDoubleClick={onDoubleClick} | ||
/> | ||
</> |
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 empty fragment wrapper <>...</>
around the Box component is unnecessary. The Box can be returned directly without the fragment wrapper.
See below for a potential fix:
<Box
sx={{
position: 'absolute',
inset: '0 -2px',
cursor: 'col-resize',
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'transparent',
transitionDelay: '0.1s',
'&:hover': {
bg: isDragging || isKeyboardDrag ? 'accent.fg' : 'neutral.muted',
},
}}
role="slider"
aria-label="Draggable pane splitter"
aria-valuemin={minWidth}
aria-valuemax={maxWidth}
aria-valuenow={currentWidth}
aria-valuetext={`Pane width ${currentWidth} pixels`}
tabIndex={0}
onMouseDown={event => {
if (event.button === 0) {
setIsDragging(true)
onDragStart?.()
}
}}
onKeyDown={event => {
if (
event.key === 'ArrowLeft' ||
event.key === 'ArrowRight' ||
event.key === 'ArrowUp' ||
event.key === 'ArrowDown'
) {
setIsKeyboardDrag(true)
onDragStart?.()
}
}}
onDoubleClick={onDoubleClick}
/>
Copilot uses AI. Check for mistakes.
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
This reverts commit 4e797ef.
This wont play well with our useSlots strategy, reverting and will fix up for next release
Closes #
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist