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.

Sign up for GitHub

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

Build popups: simplification #414

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+6 −4
Open

Build popups: simplification #414

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2efa646
Build popups: simplification
humitos Jul 4, 2024
cde841b
Add link to commit URL from the build chip
humitos Jul 4, 2024
3d38e7a
Merge branch 'main' into humitos/simplify-chips
humitos Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion readthedocsext/theme/templates/builds/partials/version_list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@
{% with build=object.last_build %}
{% if build %}
<div class="item">
{% include "includes/elements/chips/build.html" with build=build text=object.project.name %}
{% include "includes/elements/chips/build.html" with build=build notext=True %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Another parameter isn't required here, we can use text=" " to still satisfy firstof.

However, I'll note below why I think we don't want this.

</div>
{% endif %}
{% endwith %}
Expand Down
8 changes: 5 additions & 3 deletions readthedocsext/theme/templates/includes/elements/chips/build.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
instance was deleted at some point. The version name is stored on the build
in this scenario.
{% endcomment %}
{% firstof text build.get_version_name %}
{% if not notext %}
{% firstof text build.get_version_name %}
{% endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does need some text, for a couple reasons:

  • This is the main content of the .ui.label and it looks too padded/empty without this
  • The other popup cards use all this same pattern - icon, main content, and detail content

image

While I agree repeating the project name isn't helpful, it is consistent. For comparison, CircleCI also is redundant when describing builds:

image

This states the project name as the word Build looks even more redundant and isn't useful information in addition:

image

So, I think project name is best, maybe Build is okay, but this text should be something.

We should revisit patterns here later. I have a pile of work on popup cards in the milestone https://github.com/readthedocs/ext-theme/milestone/6

{% endblock chip_text %}

{% block chip_detail_text %}
Expand All @@ -30,7 +32,7 @@
{% comment %}
This block is the same as the chip_text block above.
{% endcomment %}
{% firstof text build.get_version_name %} #{{ build.id }}
Build <a href="{% url 'builds_detail' build.project.slug build.pk %}">#{{ build.id }}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be marked as translatable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, none of the other cards have linkable titles, as the chip is linked already.

For now, the full title should be linked, not just a partial title. This makes translation easier.

For later though:

  • Link does not display as blue text, this doesn't look as nice
  • We should do this at the base template so that all popupcards operate the same way.

{% endblock popupcard_header %}

{% block popupcard_right %}
Expand All @@ -53,7 +55,7 @@
{% if build.commit %}
<span>
<i class="fad fa-code-commit icon"></i>
{{ build.commit|slice:"0:8" }}
<a href="{{ build.get_commit_url }}">{{ build.commit|slice:"0:8" }}</a>
</span>
{% endif %}
<div class="right floated">
Expand Down