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

feat: Log tab enhancements #458

Merged
merged 26 commits into from
Oct 17, 2022
Merged

feat: Log tab enhancements #458

merged 26 commits into from
Oct 17, 2022

Conversation

salman-amjad
Copy link
Contributor

@salman-amjad salman-amjad commented Oct 4, 2022

  • Adds a Live button which will allow us to tail logs
  • Adds a Fullscreen option
  • Adds the ability to do a simple filter
  • Use line numbers for logs
bull-board-logs-ui-update.mov

@salman-amjad salman-amjad changed the title Feature: Log tab enhancements feat: Log tab enhancements Oct 4, 2022
@felixmosh
Copy link
Owner

Cool PR, I'll review it asap, thank you 🙏🏼

@felixmosh
Copy link
Owner

In addition there are several UX improvements,
image

  1. The board doesn't supports Dark mode (maybe we should open a new issue for it), so the logs should be with a white theme. otherwise, it catches a lot of attention.
  2. The custom scroller looks out of place, bulky, maybe it is better to make it a bit thiner.
  3. There is no spacing between the percentage loader and the logs container.
  4. The Buttons in the UI are based on icons, I think that it is better to make the Fullscreen & Live buttons as icons only
  5. The search text is white on white, (I'm using a OS dark theme)
    image

Overall, this is really great improvement to the logs section, and I'll happy to add it to the board 🙏🏼 .

I think that it is better to split these improvements into small PRs:

  1. Fullscreen capabilities
  2. Live mode when the job is active
  3. Ability to search inside logs.

pollingTimer.current = setInterval(async () => {
const logs = await actions.getJobLogs();
setLogs(getFilteredLogs(formatLogs(logs)));
el.scrollTo({ top: pre.scrollHeight });
Copy link
Owner

Choose a reason for hiding this comment

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

Why not to scroll the container to 0?

@felixmosh
Copy link
Owner

image
Some styles changes, and code refactoring

@felixmosh
Copy link
Owner

felixmosh commented Oct 13, 2022

@salman-amjad I've made some improvements and style changes, can you review this branch?

git remote -v #check if you have an upstream points to this repo
git remote add upstream [email protected]:felixmosh/bull-board.git # run to add an upstream if not exists
git fetch upsteam
git branch -b pr-458-improvments

EDIT: I've pushed the changes :]

@salman-amjad
Copy link
Contributor Author

@salman-amjad I've made some improvements and style changes, can you review this branch?

git remote -v #check if you have an upstream points to this repo
git remote add upstream [email protected]:felixmosh/bull-board.git # run to add an upstream if not exists
git fetch upsteam
git branch -b pr-458-improvments

EDIT: I've pushed the changes :]

I actually got to something pretty similar myself haha

image

@salman-amjad
Copy link
Contributor Author

@felixmosh I've copied some of your improvements from pr-458-improvments. Using <ol /> to generate line numbers doesn't seems to be working as it won't preserve the line numbers after a filter is applied

I'll probably do another PR after this one to move settings like polling time etc to settings UI

bull-board-logs-ui-update-v2.mov

@felixmosh
Copy link
Owner

felixmosh commented Oct 14, 2022

@felixmosh I've copied some of your improvements from pr-458-improvments. Using <ol /> to generate line numbers doesn't seems to be working as it won't preserve the line numbers after a filter is applied

I'll probably do another PR after this one to move settings like polling time etc to settings UI

bull-board-logs-ui-update-v2.mov

Pay attention, I'm using ol, but not the default numbering... It do preserves line number, and the benefit of this method is that all the logs are aligned (when a line number digit increases)

@salman-amjad
Copy link
Contributor Author

@felixmosh I've copied some of your improvements from pr-458-improvments. Using <ol /> to generate line numbers doesn't seems to be working as it won't preserve the line numbers after a filter is applied
I'll probably do another PR after this one to move settings like polling time etc to settings UI
bull-board-logs-ui-update-v2.mov

Pay attention, I'm using ol, but not the default numbering... It do preserves line number, and the benefit of this method is that all the logs are aligned (when a line number digit increases)

I know what that piece of code is doing and we need to use value attribute instead of data-line-number to make it work on all major browsers ( https://developer.mozilla.org/en-US/docs/Web/HTML/Element/li#attr-value ), but the issue is that it adds a lot of whitespace around some line numbers when the line numbers get up to 4+ digits length

Up to you tho, if you think it looks better with the ol plz feel free to add it back in

@felixmosh felixmosh merged commit c3e8419 into felixmosh:master Oct 17, 2022
@felixmosh
Copy link
Owner

Thank you for this PR, and the co-operation
This PR released in v4.6.0

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.

2 participants