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

Fix small issues on Joblist #3767

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chrisvanrun
Copy link
Contributor

Closes #3436

Screen.Recording.2024-12-24.at.16.28.39.mov

@chrisvanrun chrisvanrun requested a review from amickan as a code owner December 27, 2024 10:37
@chrisvanrun chrisvanrun linked an issue Dec 27, 2024 that may be closed by this pull request
@jmsmkn
Copy link
Member

jmsmkn commented Jan 6, 2025

Why is it separate? Can't the user click on "Failed" and the error message be displayed?

@chrisvanrun
Copy link
Contributor Author

Why is it separate? Can't the user click on "Failed" and the error message be displayed?

Trying to apply a somewhat consistent design: if it has an icon it is a clickable button. At the same time I aimed to keep the status badge consistent, while previewing the error message. Imho the current design is better: having the error in the results tab where there are all kinds of buttons already.

@jmsmkn
Copy link
Member

jmsmkn commented Jan 7, 2025

The use wants to know why something failed. It makes more sense to keep that information together rather than putting it in the output, which is unrelated.

@chrisvanrun
Copy link
Contributor Author

The use wants to know why something failed. It makes more sense to keep that information together rather than putting it in the output, which is unrelated.

Shall I push the Failed badge and error together to create a single badge?

@jmsmkn
Copy link
Member

jmsmkn commented Jan 7, 2025

Yes - what we had before was clicking on "Failed" and then getting a modal with the error message. It worked well.

@jmsmkn
Copy link
Member

jmsmkn commented Jan 7, 2025

Same for "Succeeded with warnings".

@chrisvanrun
Copy link
Contributor Author

Picking this up now.

@chrisvanrun
Copy link
Contributor Author

New interaction:

Screen.Recording.2025-01-10.at.13.29.53.mov

@chrisvanrun chrisvanrun requested a review from jmsmkn January 10, 2025 12:44
@chrisvanrun chrisvanrun assigned jmsmkn and unassigned chrisvanrun Jan 10, 2025
Copy link
Member

@jmsmkn jmsmkn left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of minor questions

</button>
</div>
<div class="text-left modal-body">
{{ object.stderr }}
Copy link
Member

Choose a reason for hiding this comment

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

This will dump the entirety of stderr, I think this needs to be passed through user_error like in the components application. Or maybe that work should be done when the job completes so that object.error_message is populated and these two conditional blocks can be unified?

<i class="fa fa-info-circle mr-1"></i>View Result Details
</a>
</div>
{% if object.rendered_result_text %}
Copy link
Member

Choose a reason for hiding this comment

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

Should we show a link to the result even if the rendered_result_text is empty? For algorithms that do not implement the legacy "results-json-file" this is just an empty string so the results column is empty:

Screenshot 2025-01-14 at 15 06 50

@jmsmkn jmsmkn assigned chrisvanrun and unassigned jmsmkn Jan 14, 2025
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.

Job list page a bit broken
2 participants