Skip to content

JamesC iss2481 Run Log Scrolling#258

Open
James-Cocker wants to merge 4 commits intomainfrom
JamesC-iss2481-run-log-scrolling
Open

JamesC iss2481 Run Log Scrolling#258
James-Cocker wants to merge 4 commits intomainfrom
JamesC-iss2481-run-log-scrolling

Conversation

@James-Cocker
Copy link
Contributor

Why?

Closes galasa-dev/projectmanagement#2481 with a shortened runLog container, added buttons to refresh and jump to top/ bottom.

… bottom

Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Signed-off-by: James Cocker <james.s.earth@gmail.com>
Copy link
Member

@eamansour eamansour left a comment

Choose a reason for hiding this comment

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

Looking really good overall and the changes work really well locally, just a couple of comments that would be great to address!

Comment on lines +97 to +105
export const fetchRunLog = async (runId: string) => {
let runLog;
try {
runLog = await fetchRunDetailLogs(runId);
} catch (error: any) {
runLog = 'Error fetching run log: ' + error;
}
return runLog;
};
Copy link
Member

Choose a reason for hiding this comment

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

Instead of modifying the run log's content on error, would it be better to keep the old content the same and display a Carbon error notification that tells the user that there was an error fetching the run log? https://carbondesignsystem.com/components/notification/usage/

Comment on lines +211 to +212
top: 0,
behavior: 'smooth',
Copy link
Member

Choose a reason for hiding this comment

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

Regarding accessibility: if someone wants reduced motion, using smooth would cause the scroll to slowly move to the top/bottom rather than instantly jumping to the top/bottom - see https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/At-rules/@media/prefers-reduced-motion

Can we make the scroll behaviour more reactive based on the above?

Comment on lines +183 to +188
// Reset search and filters
setSearchTerm('');
setDebouncedSearchTerm('');
setCurrentMatchIndex(-1);
setTotalMatches(0);
setSearchCache(new Map());
Copy link
Member

Choose a reason for hiding this comment

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

If I'm expecting a debug message to appear and I've searched for it in the search bar, then refreshing the run log will clear the searched term. I think it would be good to keep the search bar content and filters the same.

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.

Web UI: a scroll to bottom + a refresh button would be good when viewing the run log

2 participants