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

[WIP] fix(Timeline): render as unordered list #5176

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

francinelucca
Copy link
Contributor

@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

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: 3f15458

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.73 KB (+0.15% 🔺)
packages/react/dist/browser.umd.js 99 KB (+0.07% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-5176 October 29, 2024 19:49 Inactive
@primer-integration
Copy link

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

@primer-integration
Copy link

primer-integration bot commented Nov 5, 2024

🟢 golden-jobs completed with status success.

@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: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Nov 6, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant