Create RelativeTime component #2484
Conversation
🦋 Changeset detectedLatest commit: 02c67ea 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 |
keithamus
force-pushed
the
relative-time
branch
4 times, most recently
from
780f9e7
to
b2b54e7
Compare
size-limit report 📦
|
| }) | ||
|
|
||
| it('renders a <relative-time>', () => { | ||
| expect(render(<RelativeTime />).type).toEqual('relative-time') |
There was a problem hiding this comment.
Would it be helpful to enforce a default formatted date as a part of this elements types to match the guidance from relative-time? Specifically:
Add a element to your markup. Provide a default formatted date as the element's text content (e.g. April 1, 2014).
| expect.extend(toHaveNoViolations) | ||
|
|
||
| describe('RelativeTime', () => { | ||
| behavesAsComponent({Component: RelativeTime}) |
There was a problem hiding this comment.
Would it help to use the getElement option for a more helpful snapshot? (or just ditch the snapshot altogether 😅 )
Co-authored-by: Keith Cirkel <keithamus@users.noreply.github.com>
class props will still render out correctly
keithamus
force-pushed
the
relative-time
branch
from
62671ef
to
9ba8d95
Compare
keithamus
force-pushed
the
relative-time
branch
from
a8f5a6d
to
0adbc06
Compare
That's a bit strange! Looks like most of the diff is coming from |
|
@siddharthkp Also bear in mind for the monolith, it already loads the element and so this won't have a material impact to the monolith codebase. |
|
I believe this is now ready to go. I'd appreciate a review from one of the @primer/react-reviewers and we can merge it and release it! |
There was a problem hiding this comment.
Looks great 🚀
Thanks for walking me through the API decisions, @keithamus! I think all the props make sense, given the complexity of this component.
|
I have one concern regarding the For instance, if there's enough space in the UI, the As a side note, and from the accessibility review, using |
Acknowledged. Let's explore some options here in a follow up, as I'd like to get this shipped today and in front of engineers, and we can extend to adding support for this later (especially as there's no active areas where responsive format changes are used).
Acknowledge this too. I'll follow up in the web component itself to make sure we're accessible here, as it is best placed to resolve this. |
This adds the
RelativeTimecomponent, which uses the<relative-time>web component as a backing component.Tracking issues: https://github.com/github/primer/issues/1258, https://github.com/github/primer/issues/1347, https://github.com/github/primer/issues/1252