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

Neater UI #188

Merged
merged 80 commits into from
Oct 31, 2024
Merged

Neater UI #188

merged 80 commits into from
Oct 31, 2024

Conversation

cvzbynek
Copy link
Collaborator

Description

This PR improves the user interface for the process manager page, introducing several enhancements for a more polished, functional layout:

  1. Adjusted the message panel to maximize screen space usage.
  2. Implemented Flexbox layout for the main container to allow the process control table to expand to full width when needed.
  3. Minor CSS adjustments to improve table and button spacing, uniform button appearance, and column alignment.
  4. Logs screen revised

Fixes #170

Type of change

  • Documentation (non-breaking change that adds or improves the documentation)
  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass (eg. python -m pytest)
  • The documentation builds and looks OK (eg. python -m sphinx -b html docs docs/build)
  • Pre-commit hooks run successfully (eg. pre-commit run --all-files)

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added or an issue has been opened to tackle that in the future.

@cvzbynek cvzbynek requested a review from cc-a October 29, 2024 09:21
@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.16%. Comparing base (c99aab6) to head (b8600fd).
Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
process_manager/tables.py 88.88% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #188      +/-   ##
==========================================
- Coverage   80.23%   80.16%   -0.07%     
==========================================
  Files          28       28              
  Lines         344      353       +9     
==========================================
+ Hits          276      283       +7     
- Misses         68       70       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Thanks @cvzbynek. What a make over! Looks great. I've made a few comments where I things an be tidied up a bit. Could you also take a pass at the alignment of content in the table columns? Whilst the left align works well for the UUID column I think the others would benefit from having their headers and content centered.

process_manager/tables.py Show resolved Hide resolved
process_manager/templates/process_manager/index.html Outdated Show resolved Hide resolved
process_manager/templates/process_manager/index.html Outdated Show resolved Hide resolved
process_manager/views/pages.py Outdated Show resolved Hide resolved
# Process the log text to exclude empty lines
log_lines = [val.data.line for val in logs_response if val.data.line.strip()]

context = {"log_lines": log_lines, "log_text": "\n".join(log_lines)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems log_text is no longer used in the template context and can be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

log_lines gives more granular control for rendering each line, while log_text maintains compatibility with existing tests and code until it's fully transitioned to use log_lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other place that seems to use log_text is the test. So I think we should remove it and update the test to reflect that.

Copy link
Collaborator Author

@cvzbynek cvzbynek Oct 31, 2024

Choose a reason for hiding this comment

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

Ok, I'll remove it. Can you update the test? After you do it,I'll make a final (I hope) comit

Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably bundle it it's a simple change here to swap log_text for log_lines -

assert "log_text" in response.context

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seems ok!

@Sahil590 Sahil590 mentioned this pull request Oct 31, 2024
10 tasks
@cvzbynek cvzbynek requested a review from cc-a October 31, 2024 10:02
Copy link
Contributor

@cc-a cc-a left a comment

Choose a reason for hiding this comment

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

Ok, ready to go I think.

@cc-a
Copy link
Contributor

cc-a commented Oct 31, 2024

We have #192 which is based against this branch so I'm going to merge that in here before merging this into main.

Copy link
Contributor

@jamesturner246 jamesturner246 left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

Not important for this PR, but maybe we can start to think about moving the CSS to dedicated documents.

@cc-a cc-a enabled auto-merge October 31, 2024 11:31
@cc-a cc-a merged commit 6be9ba7 into main Oct 31, 2024
4 checks passed
@cc-a cc-a deleted the neater_ui branch October 31, 2024 11:32
#message-list {
max-height: 80vh;
overflow-y: auto;
}
.table-container {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a lot of extra css in this PR, and it's all put into the index page for the process_manager. Would it be better off that it is moved to the base.html so all the pages (in both the process_manager and the controller) inherit the same css adjustments?

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.

Process Manager Layout and Design
6 participants