-
Notifications
You must be signed in to change notification settings - Fork 188
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
Highlight new files #1610
Highlight new files #1610
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.
I really like the idea of highlighting files that are recently modified. Having the loading icon, row highlighting, and bolding the time might be a little much. Is bolding the time necessary?
@@ -40,6 +42,10 @@ function sortData(cellData, file) { | |||
return file; | |||
} | |||
|
|||
function isRecentlyModified(mtime) { | |||
return new Date().getTime() / 1000 - mtime <= RECENTLY_MODIFIED_SECONDS; |
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.
Date.now()
is equivalent to new Date().getTime()
@@ -64,18 +70,33 @@ function TaskFileBrowser (props) { | |||
return _.sortBy(props.files, 'isDirectory').reverse(); | |||
} | |||
|
|||
function recentlyModifiedTooltip() { |
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.
This doesn't have to be a function. You can create it once and assign it to a variable.
const recentlyModifiedTooltip = <Tooltip />;
<OverlayTrigger overlay={recentlyModifiedTooltip} />
return ( | ||
<div> | ||
<Breadcrumbs items={pathItems} /> | ||
<UITable | ||
data={getFiles() || []} | ||
keyGetter={(file) => file.name} | ||
rowClassName={({mtime}) => { return isRecentlyModified(mtime) ? 'bg-info-light' : null; }} |
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.
This will only update if something else causes this component to re-render. Is this the behavior that you want?
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.
The app globally refreshes this data every 60 seconds, and when the window regains focus, so this shouldn't get out of sync.
label="" | ||
id="icon" | ||
key="icon" | ||
cellData={(file) => isRecentlyModified(file.mtime) && |
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.
Because this depends on the current time, you should evaluate this once at the beginning of the render function instead of calling it 3 times.
Users want a way to determine if a file is currently being written to, especially in cases where downloading an incomplete file such as a heap dump would be undesirable.
The task file browser will highlight files that have been modified in the last 60 seconds.