-
Notifications
You must be signed in to change notification settings - Fork 360
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
UI: Commit info consistent across screens #6593
Conversation
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.
GitHub Actions / Analyze says:
'CommitMetadata' is assigned a value but never used
Really cool, seems great, and I reckon it is structured better this way. But will defer to @eladlachmi And The GUI Wizards on 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.
LGTM - small comment and the fact that some of the code is 4 space and some 2 space. Both can be handled later in a different PR.
<tr> | ||
<td><strong>Creation Date</strong></td> | ||
<td> | ||
{dayjs.unix(commit.creation_date).format("MM/DD/YYYY HH:mm:ss")} ({dayjs.unix(commit.creation_date).fromNow()}) |
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 know this is not knew but we should use localized date/time formats.
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.
we should. Feel free to open an issue :)
Thanks @ozkatz ! |
Moved the commit information component to
src/lib/components/
to make it reusable across screens.Used it to make both the commit page and the "blame" button in the objects page to show the same info in a consistent manner.
"Blame" Before:
"Blame" After: