-
Notifications
You must be signed in to change notification settings - Fork 663
Replace Timeline Playground with Custom Event playground #7836
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
Draft
janmaarten-a11y
wants to merge
10
commits into
main
Choose a base branch
from
timeline/custom-event-playground
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0218d9f
Replace Timeline Playground with Custom Event playground
janmaarten-a11y d4e8ccd
chore: auto-fix lint and formatting issues
janmaarten-a11y 61e82ff
Remove playground story id from Timeline.docs.json
janmaarten-a11y 605e7d4
Hide actorName for copilot, auto-sync for bot
janmaarten-a11y f51d7ef
Sync actorName field on every actorType change
janmaarten-a11y 1b94973
Fix axe link-name violation when actorName is empty
janmaarten-a11y d837d6e
Merge branch 'main' into timeline/custom-event-playground
janmaarten-a11y 9b175d9
Mark badge icon and large actor avatar as decorative
janmaarten-a11y e03c18f
Defensively handle hidden actorName when actorType is copilot
janmaarten-a11y f8e2e95
Remove className arg from Custom Event playground
janmaarten-a11y File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,4 +67,4 @@ | |
| "props": [] | ||
| } | ||
| ] | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Story-local styles for the Custom Event playground in Timeline.stories.tsx. | ||
| * | ||
| * The `LeftRailGutter` wrapper reserves horizontal whitespace to the left of the | ||
| * timeline rail so toggling between `actorSize: 'small'` and `actorSize: 'large'` | ||
| * does not horizontally shift the timeline. This mirrors the Rails ViewComponents | ||
| * `.TimelineItem-avatar { position: absolute; left: -72px; }` treatment WITHOUT | ||
| * adding a public avatar slot to Primer React's Timeline component (Phase 2 will | ||
| * evaluate that API change). | ||
| */ | ||
|
|
||
| .LeftRailGutter { | ||
| /* Reserve enough room to the left of the rail for a 40px avatar plus a 16px gap. */ | ||
| padding-left: calc(var(--base-size-40) + var(--base-size-16)); | ||
| } | ||
|
|
||
| .LargeActorAvatar { | ||
| position: absolute; | ||
| /* Vertically centered with the 32px badge: badge top (16px padding) + 16px half = 32px center; avatar top = 32px - 20px (half avatar) = 12px. */ | ||
| top: var(--base-size-12); | ||
| /* Matches Rails Timeline ViewComponents `.TimelineItem-avatar { left: -72px }`. */ | ||
| left: calc(-1 * (var(--base-size-40) + var(--base-size-32))); | ||
| z-index: 1; | ||
| } | ||
|
|
||
| .SmallActorAvatar { | ||
| margin-right: var(--base-size-4); | ||
| /* `vertical-align: middle` is more reliable than `text-bottom` for 20px avatars next | ||
| to body text; the slight negative nudge optically aligns the avatar to the x-height. */ | ||
| vertical-align: middle; | ||
| position: relative; | ||
| /* stylelint-disable-next-line primer/spacing -- 1px optical nudge to align avatar with text x-height */ | ||
| top: -1px; | ||
| } | ||
|
|
||
| .ActorName { | ||
| font-weight: var(--base-text-weight-semibold); | ||
| color: var(--fgColor-default); | ||
| } | ||
|
|
||
| .AppName { | ||
| font-weight: var(--base-text-weight-semibold); | ||
| color: var(--fgColor-default); | ||
| } | ||
|
|
||
| .AppAvatar { | ||
| margin-right: var(--base-size-4); | ||
| vertical-align: middle; | ||
| position: relative; | ||
| /* stylelint-disable-next-line primer/spacing -- 1px optical nudge to align avatar with text x-height */ | ||
| top: -1px; | ||
| border-radius: var(--borderRadius-medium); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How many of these are truly story-only since they look like fundamental changes? Should we be representing what things actually render as OOTB and file some follow-up issues to better align the rendering to the expected output rather than overriding locally?
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.
@adierkens 100% agree, and this is the loudest finding of the whole exercise. The fact that we need story-local CSS to fake a left-rail avatar gutter is exactly what tells us the component needs a real avatar slot. Same for the right-controls floats we left out entirely — lots of events have buttons, SHAs, and other content on the right side. The story-local CSS is deliberately scoped to this file to keep the React component untouched in this PR, so the gap stays loud rather than hidden.
That said, they're not strictly new — the large-avatar treatment mirrors how the equivalent Rails ViewComponents already render today, it just never formally made it into the React API. One of the reasons I've kept this PR as a draft is that I've been wondering whether we should land those API changes first, so when this merges it's not story-only. It's a real chicken/egg question and I'd be curious what you think.
I noted these in the wrap-up comment and on the parent issue github/primer#6662, but you're right that we should have actual followup issues. Will be filing these to scope for Phase 2:
Timeline.Item(covers the large-avatar gutter and the Copilot-avatar fallback that the Rails component has already outgrown)Octiconcleanup +BADGE_VARIANTSdedup from your other commentThere 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.
Added: