Skip to content
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

Merged
merged 4 commits into from Sep 18, 2017
Merged

Highlight new files #1610

merged 4 commits into from Sep 18, 2017

Conversation

@kwm4385
Copy link
Contributor

@kwm4385 kwm4385 commented Aug 15, 2017

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.

image

The task file browser will highlight files that have been modified in the last 60 seconds.

kwm4385 added 2 commits Aug 15, 2017
@kwm4385 kwm4385 added UI labels Aug 15, 2017
Copy link

@andyhuang91 andyhuang91 left a comment

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;

This comment has been minimized.

@andyhuang91

andyhuang91 Aug 15, 2017

Date.now() is equivalent to new Date().getTime()

@@ -64,18 +70,33 @@ function TaskFileBrowser (props) {
return _.sortBy(props.files, 'isDirectory').reverse();
}

function recentlyModifiedTooltip() {

This comment has been minimized.

@andyhuang91

andyhuang91 Aug 15, 2017

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; }}

This comment has been minimized.

@andyhuang91

andyhuang91 Aug 15, 2017

This will only update if something else causes this component to re-render. Is this the behavior that you want?

This comment has been minimized.

@kwm4385

kwm4385 Aug 15, 2017
Author Contributor

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) &&

This comment has been minimized.

@andyhuang91

andyhuang91 Aug 15, 2017

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.

kwm4385 added 2 commits Aug 15, 2017
@kwm4385 kwm4385 added hs_qa labels Aug 15, 2017
@baconmania baconmania requested a review from tpetr Sep 18, 2017
@tpetr
tpetr approved these changes Sep 18, 2017
@baconmania baconmania merged commit 1a1b256 into master Sep 18, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@baconmania baconmania deleted the highlight-new-files branch Sep 18, 2017
@baconmania baconmania added this to the 0.17.0 milestone Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.