-
Notifications
You must be signed in to change notification settings - Fork 3
Update build_name to make it a bit more explicit #615
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
base: main
Are you sure you want to change the base?
Conversation
This also refactors the logic to have 2 blocks instead of a ton of interspersed if statements, I find this much easier to read, though TBH each of these includes is only used once, so could probably be better off inline in the template instead of as an include.
I've been hoping for improvement to this as well, I don't love what is in the listing/build detail. I noted this in chat, but for context here too, the current display tried to move away from what we had before. This was on the listing and detail showing a prominent "Build #28123123". The reason for this is because the shared id sequence makes this number to big to be useful and project build ids are not sequential either. This makes it really difficult to scan the build ids or to use them at all -- for example, it takes work to tell they are even in sequential order by id: So I started drifting the build id to be more visually demoted, hoping we could remove it or find another solution. So I'd like to not go backwards here, but I would be 👍 on finding a different solution. There does still need to be difference in the display listing at least. Better forward options could be:
|
<a href="{{ build.get_absolute_url }}"> | ||
<span class="ui breadcrumb"> | ||
<span class="active section"> | ||
<span class="ui grey text"> | ||
{# Translators: this shows the build number, example "Build #1234" #} | ||
{% blocktrans trimmed with build_id=build.pk %} | ||
Build #{{ build_id }} | ||
{% endblocktrans %} | ||
</span> | ||
</span> | ||
</span> | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah as you were saying it would be better to just skip the include. With this change you aren't showing any other breadcrumbs, so we don't want to use that element anymore. And we don't want to show the text as grey anymore either, in line with all of the other listing views. It was only grey to visually demote the id.
So this becomes:
<a href="{{ build.get_absolute_url }}"> | |
<span class="ui breadcrumb"> | |
<span class="active section"> | |
<span class="ui grey text"> | |
{# Translators: this shows the build number, example "Build #1234" #} | |
{% blocktrans trimmed with build_id=build.pk %} | |
Build #{{ build_id }} | |
{% endblocktrans %} | |
</span> | |
</span> | |
</span> | |
</a> | |
<a href="{{ build.get_absolute_url }}"> | |
{# Translators: this shows the build number, example "Build #1234" #} | |
{% blocktrans trimmed with build_id=build.pk %} | |
Build #{{ build_id }} | |
{% endblocktrans %} | |
</a> |
@@ -64,7 +64,7 @@ | |||
data-bind="semanticui: { popup: { content: '{{ object.date }}', position: 'top center', delay: { show: 500 }, variation: 'small'}}"> | |||
{# Translators: this will read like "Started 1 month, 3 days ago" #} | |||
{% blocktrans with object.date|naturaltime as date trimmed %} | |||
Started {{ date }} | |||
{{ date }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and not sure on the reason for removal here, but this feels too terse without prefix text. There should be some context around this date. "5 minutes ago" is ambigious -- started 5 minutes ago or finished 5 minutes ago?
This also refactors the logic to have 2 blocks instead of a ton of interspersed if statements,
I find this much easier to read,
though TBH each of these includes is only used once,
so could probably be better off inline in the template instead of as an include.