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

Revamp log viewer for apps ran [KBASE-5405] #1215

Merged
merged 20 commits into from
Nov 29, 2017
Merged

Conversation

tiramisu24
Copy link
Contributor

-add scroll function
-nav buttons scroll instead of page
-will only fetch previous log lines on page up or scroll up
-change style of errors
-can expand out the view

@coveralls
Copy link

Coverage Status

Coverage increased (+4.6%) to 12.477% when pulling 542ae99 on tiramisu24:develop into ff03122 on kbase:develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+4.6%) to 12.477% when pulling 7d16716 on tiramisu24:develop into ff03122 on kbase:develop.

Copy link
Member

@briehl briehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the scrolling still looks a little off, and possibly confusing, but this is a good first step. I like that the arrow buttons reflect the direction of the scrolling now.

Just a few stylistic changes that should get fixed before merging, then we can merge and iterate on the scrolling part.

if(target.is(':last-child')){
target.show();
}else{
// target.show('slide', {direction: 'up'});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go ahead and delete commented code. that's what git's for. :)

// target.show('slide', {direction: 'up'});
target.slideDown();
}
// $panel.prepend(target);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

.on('scroll', function () {

//at begining
var top = $(this).scrollTop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs semicolon

@@ -943,7 +1060,7 @@ define([
ui.hideElement(element);
});
}

// Emit messages for this state.
// if (state.ui.messages) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more deletion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of what?

@@ -1005,6 +1122,8 @@ define([
fsm.bus.on('on-job-not-found', function(message) {
doJobNotFound(message);
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs another semicolon

return lines.map(function(line) {
return renderLine(line);
}).join('\n');
var $section = $('<div/>')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs semicolon

@coveralls
Copy link

Coverage Status

Coverage increased (+4.7%) to 12.512% when pulling 6b463a1 on tiramisu24:develop into ff03122 on kbase:develop.

@briehl briehl merged commit 383e5f7 into kbase:develop Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants