-
Notifications
You must be signed in to change notification settings - Fork 393
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(logs): drawer with details #2155
Conversation
Next PRs:
|
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.
Love the UI, great work. 👏
The search operation feature is not working yet. right?
From a layout point of view my only feedback is that the status button seems a bit out of place next to the search bar.
res.status(400).send({ | ||
error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) } | ||
}); | ||
return; |
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.
those 2 checks come back regularly. I imagine we could abstract them
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 using your wrapper might be the best solution because I could not come up with an abstraction that save more than 1 line (you still need to return something so you still need a if)
but if you have something in the meantime, happy to change
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 don't have a concrete solution right now. my comment was more to keep it in the back on our mind so at some point we come up with something
const body: SearchMessages['Body'] = val.data; | ||
const rawOps = await model.listMessages({ | ||
parentId: body.operationId, | ||
limit: body.limit!, |
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 wonder if there is a way make the type reflect the fact that after validation, since limit has a default value it cannot be undefined anymore
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.
yes it's a bit annoying, with types you often need to produce multiple variants: before/after validation, before/after creation, etc.
I'm not sure if there is a good answer expect maybe having a Body
variant in the Endpoint types that you can alter to reflect post-validation type.
Yes sorry, I added the UI componennt and realized it was more complicated than anticipated |
<Route path="/:env/environment-settings" element={<EnvironmentSettings />} /> | ||
<Route path="/:env/project-settings" element={<Navigate to="/environment-settings" />} /> | ||
}} | ||
> |
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.
hard to tell what this is changing....what is the adjustment here?
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.
return ( | ||
<div className="py-6 px-6 flex flex-col gap-9"> | ||
<h3 className="text-xl font-semibold text-white flex gap-4 items-center">Operation Details</h3> | ||
<Skeleton className="h-4 w-[250px]" /> |
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.
If the Skeleton
spacing needs to change seems it'll be annoying to change it in all these different places
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.
by spacing you mean height?
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, height and weight. My comment should have said:
If the Skeleton
element needs to change
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.
Skeleton is made to be flexible to reflect the final UI so it's bound to be modified where it needs to. I'll set the default height which is more common but the width is context specific.
For the ref, this is that: https://ui.shadcn.com/docs/components/skeleton
<div className="font-semibold text-sm">Connection</div> | ||
<div className="text-gray-400 text-s font-code truncate"> | ||
{operation.connectionName ? ( | ||
<Link to={`/connections/${operation.connectionName}`} target="_blank" className="flex gap-1 items-center hover:text-white"> |
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.
Should it be a new link? I'm not sure, asking from a product perspective though
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.
a new link?
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.
new link === opens in a new tab
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.
ah yes sorry I see. The main inspiration is datadog drawer so for now it's the intended behavior to not loose the context. There are plenty of stuff in the UX that will evolve with the first feedback
}, | ||
width: { | ||
largebox: '1200px', | ||
largecell: '480px' | ||
}, | ||
fontSize: { | ||
s: '13px', |
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.
14px is also used a lot, seems a bit strange for things to go to 13px. Something to confirm with the design I suppose
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.
Yes, we increased the font-size but I think 14 was too big. You can find the figma in Product > Logs V2
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.
Looks great 😍, excited to test it out once it is wired up.
Describe your changes
Fixes NAN-847
Contributes to NAN-908
Rename
POST /logs/search
toPOST /logs/operations
It was too different after all for a unified endpoint
Create
POST /logs/messages
to search in messages tied to an operationImplement Operation's Drawer
Implement Messages's Drawer
Full text search on logs
http://localhost:3000/dev/logs
Screen.Recording.2024-05-21.at.11.13.23.mp4