-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add infrastructure tabs and content #2588
Conversation
ee9bf31
to
6e70668
Compare
playwright-report/ | ||
test-results/ | ||
|
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.
thanks for catching this!
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.
You bet! I think there are going to be other similar issues after the relocating of all these files.
className?: string | ||
} | ||
|
||
export const List = <T,>({ items, renderItem, onClick, className }: ListProps<T>) => { |
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.
Did you mean to remove this ,
or is that doing some fancy JS magic I haven't learned yet? :)
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.
I think the ,
is required for the component to parse correctly. Removing it causes all kinds of errors 😂 . I think it's just a react generic component feature.
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.
oh wow that's good to know!
<li | ||
key={index} | ||
className={`relative flex justify-between gap-x-6 p-4 ${onClick ? 'cursor-pointer hover:bg-gray-50 dark:hover:bg-gray-700' : ''}`} | ||
onClick={onClick ? () => onClick(item) : undefined} |
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.
Just to confirm, if you use this component without an onClick
, does passing undefined
into this prop definitely not throw a runtime error? I wasn't sure if it was safe to pass undefined
to a fn
prop.
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.
yeah should be ok. All the current stuff is using it without an onClick
.
<div className='flex gap-x-4 items-center w-1/2'> | ||
<div className='whitespace-nowrap'> | ||
<div className='flex gap-x-2 items-center'>{route.module}</div> | ||
<p className='mt-1 flex text-xs leading-5 text-gray-400 font-roboto-mono'>{route.endpoint}</p> |
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.
nit: this shade of gray feels good for dark mode but a little eye-strainy for light mode. What do you think about bumping it up to 600 when mode=light?
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.
Bumped to 500
which feels pretty good too
idle: 'text-gray-500 bg-gray-100/10', | ||
success: 'text-green-400 bg-green-400/10', | ||
error: 'text-rose-400 bg-rose-400/10', |
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.
I feel like these also look great on dark mode but feel a little light for light mode. What do you think?
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.
Good call! Updated.
6e70668
to
78c6c0f
Compare
GLORIOUS! |
Fixes #2552