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

[UI] Batches UI Improvements #14640

Merged
merged 5 commits into from
Aug 19, 2024
Merged

Conversation

cjllanwarne
Copy link
Collaborator

@cjllanwarne cjllanwarne commented Jul 29, 2024

Screenshots should give the main overview of the changes
Questions for reviewers.

Technical:

  • Are there any CSS conventions within Hail? I assume I need to migrate the ad-hoc "style" tags into CSS?
  • There still seems to be a bunch of unused space after truncated batch names. I'm not sure why

UX:

  • I've moved the status indicator to the front of the line. Is that ok?
    • to help with layout within the batch-name box
    • to put it in a reliable place (ie not moving around based on how long the name is)
  • I'm not really sure I like the change to Pending. Curious for others' thoughts

Example: Batches page

(layout and columns)

Before:
image
After:
image

Example: Batch Details page

(Renaming confusing 'Pending' field)

Before:
image
After:
image

Fixes #14628. Adds and shuffles content on the new Batches table

@cjllanwarne
Copy link
Collaborator Author

@jmarshall curious for your thoughts on these example screenshots. Does this address your concerns from the original ticket?

@cjllanwarne cjllanwarne marked this pull request as ready for review August 7, 2024 17:51
Copy link
Collaborator

Are there any CSS conventions within Hail? I assume I need to migrate the ad-hoc "style" tags into CSS?

See the PR #14562 where Daniel introduced the new UI. He decided to use tailwind, which apparently (I don't know much about frontend stuff) prefers to keep styling in the html.

I've moved the status indicator to the front of the line. Is that ok?

I like it.

I'm not really sure I like the change to Pending. Curious for others' thoughts

In the template do we have the number of running jobs available? If so maybe we split "Pending" into "Pending" and "Running". Otherwise I think this is fine.

Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

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

These are all great changes

Copy link
Collaborator

@chrisvittal chrisvittal 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 this improves UI clarity. Nice change!

@@ -46,7 +46,7 @@

{% call collapsible_li(true, 'Jobs', batch['n_jobs']) %}
{{ kv_table({
'Pending': batch['n_jobs'] - batch['n_completed'],
'Incomplete (Blocked, Queued or Running)': batch['n_jobs'] - batch['n_completed'],
Copy link
Collaborator

@chrisvittal chrisvittal Aug 15, 2024

Choose a reason for hiding this comment

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

I like this. To me, 'Pending' implies 'not running', so this improves clarity.

@hail-ci-robot hail-ci-robot merged commit ec3e89f into hail-is:main Aug 19, 2024
6 checks passed
@cjllanwarne cjllanwarne deleted the cjl_batches_ui branch August 19, 2024 20:59
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.

[batch] New UI friction points
4 participants