Skip to content

Conversation

humitos
Copy link
Member

@humitos humitos commented Jul 7, 2025

I find pretty hard to know what's the version of the current build on the build details' page, so I want to make it more visible.

I'm following the same style/pattern we are following for the build id and I like it.

Screenshot_2025-07-07_12-50-18

@humitos humitos requested a review from a team as a code owner July 7, 2025 10:52
@humitos humitos requested a review from agjohnson July 7, 2025 10:52
I find pretty hard to know what's the version of the current build on the build
details' page, so I want to make it more visible.

I'm following the same style/pattern we are following for the build id and I
like it.
@humitos humitos force-pushed the humitos/detach-version-name branch from 0395f1a to 15a1700 Compare July 8, 2025 08:57
@humitos
Copy link
Member Author

humitos commented Jul 8, 2025

This is how it looks with the latest changes.

Versions listing page

Screenshot_2025-07-08_11-00-25

Build details page

Screenshot_2025-07-08_11-01-09

I like it 👍🏼 . Let me know if you have any other feedback here.

@humitos humitos requested a review from agjohnson July 8, 2025 09:02
@agjohnson
Copy link
Contributor

I didn't think about the listing view, I don't think this quite works there. As a breadcrumb, Version has gone from a descriptor of the version type to just a navigation item.

The reason the verbose name of the version type is in there is to differentiate between versions and pull request builds:

image

What you have here is going to make one style for versions (Versions / {name} / #1234) and a separate style for pull requests (Pull request #999 / #1235). I think we should stick with consistency here and not use the extra breadcrumb section in the listing view -- that is, keep what we have now on the listing view.

The header change mostly works on the build detail view though, but I'd suggest we always have the extra Versions breadcrumb for navigation:

  • Versions > {name} > Builds > #1234
  • Versions > Pull request 999 > Builds > #1235

Comment on lines +78 to +80
{% if is_page_title %}<a href="{{ build.get_absolute_url }}">{% endif %}
<span class="ui grey text">#{{ build.pk }}</span>
{% if is_page_title %}</a>{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

djlint isn't picking up on the conditional start/end tags and is breaking indenting. I believe in other templates we use {# fmt:off #}/fmt:on comments around these conditionals to fix this.

@humitos
Copy link
Member Author

humitos commented Jul 9, 2025

This is how it's looking now. Looks great to me!

Screenshot_2025-07-09_13-12-45


Screenshot_2025-07-09_13-13-02


Screenshot_2025-07-09_13-12-53

@agjohnson
Copy link
Contributor

agjohnson commented Jul 9, 2025

image

The build listing is still off. I don't think we should put a breadcrumb like this in the build list, from my comment here:

For consistency that should still be Version latest and Pull request 999. It's too noisy with the Versions breadcrumb in every line.

It's probably easiest overall to just drop the include and have the build listing display what it was previously and the build detail page to use what you have here. The build detail page looks good now, the listing view is the only issue. This would also avoid the djlint formatting issue I noted.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

And some small adjustments, my main feedback is still at:

Comment on lines +54 to +55
<span class="section">
<span class="divider">/</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ordering is off here, see https://fomantic-ui.com/collections/breadcrumb.html

Suggested change
<span class="section">
<span class="divider">/</span>
<span class="divider">/</span>
<span class="section">

Comment on lines +64 to +65
<span class="section">
<span class="divider">/</span>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<span class="section">
<span class="divider">/</span>
<span class="divider">/</span>
<span class="section">

@humitos
Copy link
Member Author

humitos commented Jul 10, 2025

For consistency that should still be Version latest and Pull request 999. It's too noisy with the Versions breadcrumb in every line.

I tend to agree here. However, the motivation of this PR was to visually distinct the word "Version" from the name of the version itself. So, if we are removing the breadcrumb here I would like to use a different color at least for the name of the version.

An idea:

Screenshot_2025-07-10_11-52-51

  • It uses code for the version name
  • It uses grey color for it

I want to highlight the name of the version somehow over the other elements of that row.

@humitos humitos requested a review from agjohnson July 15, 2025 10:27
@agjohnson
Copy link
Contributor

This breaks styling in a few ways though, and I'm not really sure the version names sticking out on the listing view header is even a problem. I mentioned that using .ui.grey.text breaks links style a couple times, but this type of change also breaks styling consistency from all other listing views. I'd rather we're not altering the look and feel listing view to listing view.

The chip concept was meant to fill this need for a prominent, succinct UI element. The build list already has a version chip for this:

image

Again, the breadcrumb and header view looks good, I'm only -1 on altering the listing view

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.

2 participants