-
Notifications
You must be signed in to change notification settings - Fork 566
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
feat(Timeline): Convert Timeline to CSS modules behind team feature flag #5329
Conversation
👋 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! |
size-limit report 📦
|
/* TODOl not quite sure if this is the correct migration for this line */ | ||
background-color: var(--timelineBadge-bgColor); |
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.
Can I get a check on this line? I wasn't quite sure what the correct mapping was here. i pulled the --timelineBadge-bgColor
from the computer styles in production
it('should support `className` on the outermost element', () => { | ||
const Element = () => <Timeline.Badge className={'test-class-name'} /> | ||
const FeatureFlagElement = () => { | ||
return ( | ||
<FeatureFlags | ||
flags={{ | ||
primer_react_css_modules_team: true, | ||
primer_react_css_modules_staff: true, | ||
primer_react_css_modules_ga: true, | ||
}} | ||
> | ||
<Element /> | ||
</FeatureFlags> | ||
) | ||
} | ||
expect(HTMLRender(<FeatureFlagElement />).container.firstChild?.firstChild).toHaveClass('test-class-name') | ||
}) | ||
}) |
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 sx-compliant Timeline.Badge doesn't support className, I thought it'd make sense to add it to the css modules refactor and therefore I'm only testing for that one here, but lmk if that doesn't make sense and I can revert!
/** | ||
* @deprecated Use the `TimelineItemProps` type instead | ||
*/ | ||
export type TimelineItemsProps = StyledTimelineItemProps & HTMLProps<HTMLDivElement> | ||
|
||
export type TimelineItemProps = StyledTimelineItemProps & HTMLProps<HTMLDivElement> |
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.
It seems to me the "correct" prop name would be TimelineItemProps
singular, so I'm proposing deprecating TimelineItemsProps
in favor of TimelineItemProps
. Would love to hear thoughts on this one
🦋 Changeset detectedLatest commit: db6b3eb 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 |
…er/react into 4366-update-timeline-to-css-modules
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/352333 |
🟢 golden-jobs completed with status |
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.
Just a few nitpicks! The color vars look good 👍
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
Co-authored-by: Katie Langerman <18661030+langermank@users.noreply.github.com>
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
1 similar comment
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
Closes https://github.com/github/primer/issues/4366
Changelog
New
Timeline
dev story to test sx and style props with VRTTimeline
module cssTimeline.Break
andTimeline.Body
className
support tests for allTimeline
componentsChanged
Timeline
e2e tests and add new dev storyTimeline
to use CSS modules when team feature flag is turned onTimelineItemsProps
in favor ofTimelineItemProps
Rollout strategy
Testing & Reviewing
Merge checklist