-
Notifications
You must be signed in to change notification settings - Fork 610
Remove the CSS modules feature flag from Pagination component #5997
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
🦋 Changeset detectedLatest commit: 8c53016 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
👋 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! |
@@ -196,53 +68,22 @@ function usePaginationPages({ | |||
} | |||
|
|||
return ( | |||
<Page {...props} key={key} theme={theme} className={clsx(enabled && classes.Page)}> | |||
<span {...props} key={key} className={clsx(classes.Page)}> |
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 thought this should have been an a
tag based on the previous code, but that caused a test to fail with the expected number of a
tags to render. Not sure if the test had to be updated, or if this is the correct default element.
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 removes the legacy CSS modules feature flag from the Pagination
component and replaces the styled‑components implementation with CSS module classes and a toggleable SX component.
- Removed
toggleStyledComponent
,styled-components
imports,useFeatureFlag
, and theme props - Replaced custom
<Page>
styled anchor with a<span>
using CSS module classes - Swapped
toggleStyledComponent
fortoggleSxComponent
on the container and always apply CSS module classes viaclassName
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/react/src/Pagination/Pagination.tsx | Remove styled‑components and feature flag, apply CSS modules classes, update container to SX component |
.changeset/nasty-nails-appear.md | Update changelog to reflect removal of CSS modules flag |
<span {...props} key={key} className={clsx(classes.Page)}> | ||
{content} | ||
</Page> | ||
</span> |
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.
Replacing the styled anchor component with a removes native anchor semantics (e.g., href support, keyboard focus, and accessibility). Consider rendering an or using a component that preserves the original element type via the as
prop.
Copilot uses AI. Check for mistakes.
size-limit report 📦
|
const PaginationContainer = toggleSxComponent('nav') as React.ComponentType< | ||
React.HTMLAttributes<HTMLElement> & SxProp & React.PropsWithChildren | ||
> |
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.
One option for these is to use BoxWithFallback
directly where the component is used since we don't really need these wrappers anymore. For example:
// Before
return (
<PaginationContainer
className={clsx(enabled && classes.PaginationContainer)}
aria-label="Pagination"
{...rest}
theme={theme}
>
// After
return (
<BoxWithFallback
as="nav"
className={clsx(enabled && classes.PaginationContainer)}
aria-label="Pagination"
{...rest}
>
Let me know what you think!
|
||
export type PaginationProps = { | ||
theme?: Record<string, unknown> |
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 think if we're exposing theme
/ it's in our Public API we should add a deprecation note here for this type and then update the usage below to noop like:
function Pagination({
theme: _theme,
This will help out when rolling this out to avoid:
- A TypeScript error with
theme
being provided (but it's no longer on the type) - A runtime error if
theme
is now added to the element that{...rest}
is being called on since it would be in therest
argument
Hope that makes sense, let me know what you think! If all of this is internal feel free to ignore me too 😂
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/376831 |
🟢 golden-jobs completed with status |
Co-authored-by: Marie Lucca <40550942+francinelucca@users.noreply.github.com>
Co-authored-by: Marie Lucca <40550942+francinelucca@users.noreply.github.com>
Co-authored-by: Marie Lucca <40550942+francinelucca@users.noreply.github.com>
Changelog
New
Changed
Removed
CSS modules feature flag from Pagination component
Rollout strategy
Testing & Reviewing
Merge checklist