Scroll fixing #1600
Scroll fixing #1600
Conversation
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "config": "dev", | |||
There was a problem hiding this comment.
Probably shouldn't check in config file changes.
| container.innerHTML = layout.content; | ||
| layout.events.attachEvents(container); | ||
| panel = document.getElementsByClassName(jobId)[0] |
There was a problem hiding this comment.
it would be better to put the job id on a data- element; e.g. data-job-id.
But there is an even better method. You see, since the scope of the Dom query is the entire document, you have know way of knowing whether there is another node with class={jobId} or even data-foo={jobId}. The only way to ensure that you get the one element you want is to use a unique value, like a uuid, and the best performing way to set it is as the element's id. You would store the uuid within the component (or widget in this case). So something like this.panelId = html.genId(), and then id: this.panelId in the markup function.
The html.genId() function generates a v4 uuid and prefixes it with html_. Element ids can't start with a number, and uuids may.
There was a problem hiding this comment.
This is especially important, since it's possible for multiple job log viewers to exist on the page, all looking at the same job id. So that query will grab all of those elements with the same job id and operate on them, instead of the single current element that you expect.
| const kblogLine = document.createElement('div') | ||
| kblogLine.setAttribute('class', 'kblog-line' + errorClass); | ||
| // kblog-num-wrapper div | ||
| const warpperDiv = document.createElement('div'); |
| }; | ||
| }); | ||
| $panel = $(ui.getElements('panel')[0]); | ||
|
|
||
| var autoState = fsm.getCurrentState().state.auto; | ||
| if (!autoState){ |
There was a problem hiding this comment.
This seems redundant with the else case below. You could do something like:
makeLogChunkDiv(viewLines);
if (autoState) {
const lastChildElement = ...
... do other stuff ...
}
There was a problem hiding this comment.
Okay, I have it running locally against appdev, here are some comments:
In general the buttons do what they are supposed to, the logs update and scroll nicely!
sending state
maximize button should be disabled when the logs are disabled (sending mode)
there should be a message below the log buttons indicating that the logs will not be available until the job is running
queued state
Job Status gets stuck showing "Determining Job State...", even after the job has been queued
You need to click the View Configure button, then back to Job Status in order for the panel to reflect that the job is queued.
The Job Status and Job Log sections are not shown when the job is queued (even though they are when the job is sending.) They should be, and the Job Log should indicate that the logs will be available when the job starts running.
running state:
I'm not sure about the behavior when the job is running but the logs are still polling. I think it could be frustrating if the user scrolls around in the log but it scrolls down to the bottom. What about continuing the polling, but not the auto-scrolling, if the user has scrolled. Then when if the user scrolls to the bottom, the auto scrolling can continue (if polling is active.)
Log lines which overflow should be aligned within the same column, currently the align at the beginning of the line, underneath the line number.
Scrolling around while the log is playing can lead to mixed up lines. I haven't looked at the precise behavior, but I think it is also related to not resetting the log between runs.
canceled
after canceling while in play mode, the log will be updated but not scrolled to the bottom.
finished
works great
errored
need to find job which ends in error!
|
I turned our collective comments into issues for further work. Gonna merge this and work on an update. Thanks, @FlyingUnicornSF !! |
@briehl, @eapearson please review