Timeline component #89
Conversation
|
@tulup-conner @bacali95 this one is a big one... May we have +1 review at least? |
| <div data-testid="timeline-point" className={classNames({ 'flex items-center': horizontal })} {...props}> | ||
| {children} | ||
| {Icon ? ( | ||
| <span className="absolute -left-3 flex h-6 w-6 items-center justify-center rounded-full bg-blue-200 ring-8 ring-white dark:bg-blue-900 dark:ring-gray-900"> |
There was a problem hiding this comment.
This needs to either not be visible to screen readers, or pass in an accessible name
There was a problem hiding this comment.
This needs to either not be visible to screen readers, or pass in an accessible name
You might have to give me hand on this one. I'm not sure what the best way to do this is. :(
There was a problem hiding this comment.
I would probably just add the sr-only class (hide this icon for screen readers) since each icon will be accompanied by relevant text already.
There was a problem hiding this comment.
I would probably just add the sr-only class (hide this icon for screen readers) since each icon will be accompanied by relevant text already.
Okay so do I just add the sr-only class and make it a conditional based on whether or not there is an Icon. This would also replace the ternary statement right?
|
|
||
| export const TimelineTitle: FC<TimelineTitleProps> = ({ children, ...props }) => { | ||
| return ( | ||
| <h3 className="text-lg font-semibold text-gray-900 dark:text-white" {...props}> |
There was a problem hiding this comment.
This needs to be morphable into any heading level
There was a problem hiding this comment.
This needs to be morphable into any heading level
Should I make a prop called tag that is passed to the heading level. The user can then only pass in a range from 1-6 to the tag prop?
There was a problem hiding this comment.
Its a pretty common pattern to provide an as prop, like:
as="h5"
|
Sorry, I created confusion by suggesting sr-only where I meant aria-hidden="true" which have the opposite effect 😂 |
|
I think this PR is ready to go, do you have other requests @rluders @tulup-conner? |
Ref: This PR is related to issue #12