Skip to content
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

fix(Timeline): render as unordered list #5176

Conversation

francinelucca
Copy link
Member

@francinelucca francinelucca commented Oct 25, 2024

Closes https://github.com/github/primer/issues/3354

Currently the Timeline component lacks semantic structure, in that it renders like a div with divs inside of it. This doesn't convey appropriate structural meaning accessibility-wise. This PR introduces a new API under a FF to have the Timeline render as an unordered list with listitems inside of it

Introduces a new rendering behavior for Timeline under feature flag:

  • if FF is off (default):
    • Timeline renders as per usual: as a div with inner divs
  • if FF is on:
    • Timeline renders as per usual: as a div
    • Timeline.Item renders as an li
    • It is expected that all Timeline.Item will be wrapped inside a Timeline.Group (new component) that renders as an ul

Changelog

New

  • primer_react_timeline_as_list feature flag (defaults to false)
  • New Timeline.Group subcomponent (used to wrap Timeline.Items within a list when FF is on
  • Add new Timeline.Group subcomponent to Timeline *.docs.json
  • Tests for new Timeline behaviors

Changed

  • Turn on new primer_react_timeline_as_list FF in SB Timeline stories, use new API
  • Added css styles to Timeline to avoid visual changes when switching to ul
  • Render TimelineItem as li if primer_react_timeline_as_list FF is on (div otherwise)
  • Add aria-hidden to Timeline.Break if primer_react_timeline_as_list FF is on
  • Update Timeline snapshot
  • Updated TimelineItem types

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

1- Release new API under FF (this PR)
2 - Move dotcom to new API (upcoming separate PR, after this is merged)
3 - Deprecate "old" API
4 - Remove old API in next major

Testing & Reviewing

Revise deployment stories, see DOM renders as list and no visual regressions

Merge checklist

Copy link

changeset-bot bot commented Oct 25, 2024

🦋 Changeset detected

Latest commit: 05c6b08

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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

Copy link
Contributor

👋 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!

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Oct 25, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-5176 October 25, 2024 19:54 Inactive
Copy link
Contributor

github-actions bot commented Oct 25, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 98.63 KB (+0.14% 🔺)
packages/react/dist/browser.umd.js 99.05 KB (+0.25% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-5176 November 8, 2024 03:07 Inactive
@francinelucca francinelucca added the staff Author is a staff member label Nov 8, 2024
@github-actions github-actions bot temporarily deployed to storybook-preview-5176 November 8, 2024 03:12 Inactive
@github-actions github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh labels Nov 8, 2024
…g' of github.com:primer/react into francinelucca/3354-prctimeline-list-structure-is-missing
Comment on lines +87 to +88
// eslint-disable-next-line @typescript-eslint/no-explicit-any
function TimelineItem<As extends React.ElementType>(props: TimelineItemsProps<As>, ref: ForwardedRef<any>) {
Copy link
Member Author

@francinelucca francinelucca Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a page out of Branchname here, open to suggestions

@primer-integration
Copy link

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/350532

@github-actions github-actions bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: failing Changes in this PR cause breaking changes in gh/gh labels Nov 8, 2024
@francinelucca francinelucca marked this pull request as ready for review November 8, 2024 18:05
@francinelucca francinelucca requested a review from a team as a code owner November 8, 2024 18:05
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay! Just wanted to ask, would it be possible for the behavior of Timeline.Group to live inside of Timeline? Basically have the flag turn Timeline into a ul and Timeline.Item into an li

If I remember right, when we last talked about this that would have a problem when we needed to make separate lists, is that right? If that's the case, could we recommend folks do multiple timelines?

My only hesitation with Timeline.Group is that it seems like it would always be necessary which made me think if we could have it baked into Timeline or not. Let me know what you think!

@francinelucca
Copy link
Member Author

If I remember right, when we last talked about this that would have a problem when we needed to make separate lists, is that right? If that's the case, could we recommend folks do multiple timelines?

Yeah I think we can do that, the thing is we need Timeline.Break outside of the list, so to replicate something like this, with that approach the code would look like this:

 <Timeline>
      <Timeline.Item>
        <Timeline.Badge sx={{
        bg: 'done.emphasis'
      }}>
          <Octicon icon={GitMergeIcon} color="fg.onEmphasis" aria-label="Merged" />
        </Timeline.Badge>
        <Timeline.Body>This is a message</Timeline.Body>
      </Timeline.Item>
</Timeline>
<Timeline.Break />
<Timeline>
    <Timeline.Item>
      <Timeline.Badge>
        <Octicon icon={GitBranchIcon} aria-label="Branch" />
      </Timeline.Badge>
      <Timeline.Body>This is a message</Timeline.Body>
    </Timeline.Item>
</Timeline>

I originally added the Timeline.Group because I just thought it looked more "neat" to have it all under one top-level element, but if that looks good to you I'm happy to implement it this way instead @joshblack

@francinelucca
Copy link
Member Author

Decided to close issue, closing PR

@francinelucca francinelucca deleted the francinelucca/3354-prctimeline-list-structure-is-missing branch December 10, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh staff Author is a staff member status: review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants