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

More job details in pm #3700

Merged
merged 7 commits into from
Aug 20, 2024
Merged

More job details in pm #3700

merged 7 commits into from
Aug 20, 2024

Conversation

ashton22305
Copy link
Contributor

Work on #3686

Renders each job as a clickable pill that expands to reveal job information.

This will need to be updated to refine the format of the data, provide more information, and prevent expanded cards from disappearing every time that the turbo stream updates

@ashton22305
Copy link
Contributor Author

There isn't a way to provide any information beyond the cluster and job id until #3685 is fixed because, when we query for a completed job, that is the only information that is returned.

@johrstrom
Copy link
Contributor

There isn't a way to provide any information beyond the cluster and job id until #3685 is fixed

That's fine, we can provide placeholders at least just to see what it'll be visually.

OR we can just hold off on this until that's complete. I'd probably choose the latter, but I'll leave it to you to decide.

@ashton22305
Copy link
Contributor Author

I have #3702 up so once it gets merged I'll make changes here.

@johrstrom
Copy link
Contributor

This is an OK first start, but I am wondering about what to change, and indeed, how to change it.

First for that large case statement - I wonder if https://docs.ruby-lang.org/en/master/Enumerable.html#method-i-zip can help us here.

Secondly, I'm wondering about presentation and if tables are indeed the way we want to go. I see the case statement is largely driven by how it's presented, so changing the presentation is likely to change the inner implementation. I'm wondering about the tables for accessibility reasons - if we want just 1 table or a list or similar.

So I just need a second to sit on this because I'm not sure if want to merge now knowing we want lots of changes or get a better foundation now.

<%= job.id %> <%= status_text(job.status.to_s) %>
</span>
</button>
<div class="collapse" id="<%= id %>Data">
Copy link
Contributor

Choose a reason for hiding this comment

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

We should keep html ids snake_case.

Comment on lines 16 to 25
<table class="table table-bordered table-sm m-0 mb-2">
<thead><%= category_name %></thead>
<% category.each do |name, value| %>
<tr>
<td><strong><%= name %></strong></td>
<td><%= value %></td>
</tr>
<% end %>
<% end %>
</table>
Copy link
Contributor

Choose a reason for hiding this comment

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

OK - I think I've got this now. I think this should just be 1 table instead of 3. I think that'll simplify the computation for this object as well.

Comment on lines 25 to 27
case key
when 'job_name'
account_info['Job Name'] = value
Copy link
Contributor

Choose a reason for hiding this comment

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

With just 1 table, this should simplify a lot. But in any case, I'm not in love with the giant case statement. Maybe try a lookup table to map the key job_name to the new key Job Name. You may even get by with a simple map to key.humanize.

Also I'd say put this somewhere as a member of the HpcJob class so we can just do job.to_human_display or something similar.

@johrstrom johrstrom self-requested a review August 20, 2024 19:02
@johrstrom johrstrom merged commit 4b09edb into master Aug 20, 2024
25 of 26 checks passed
@johrstrom johrstrom deleted the more-job-details branch August 20, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants