Update build list and detail page UX #5916
Conversation
saadmk11
added
the
PR: work in progress
| <a href="{{ build.version.get_external_version_url }}">{{ build.version.verbose_name }}</a> | ||
| {% else %} | ||
| {{ build.version.slug }} | ||
| {% endif %} | ||
| {% endif %} | ||
| </span> | ||
| <span class="build-commit" |
There was a problem hiding this comment.
I'd also love to link the commit as well, using the same pattern.
saadmk11
removed
the
PR: work in progress
| try: | ||
| if self.project.remote_repository.account.provider == 'github': | ||
| return GITHUB_EXTERNAL_VERSION_NAME | ||
| except Exception: |
There was a problem hiding this comment.
We should log this exception, otherwise it will hide real issues. What exception are we really trying to catch?
readthedocs/builds/models.py
Outdated
| Branch/Tag Example: https://github.com/rtfd/readthedocs.org/tree/3.4.2/. | ||
| Pull/merge Request Example: https://github.com/rtfd/readthedocs.org/pull/9999/. | ||
| Example: https://github.com/rtfd/readthedocs.org/tree/3.4.2/. | ||
| External Version Example: https://github.com/rtfd/readthedocs.org/pull/99/commits/b630b630/. |
There was a problem hiding this comment.
Don't we want this URL to point to the PR still? Otherwise we are changing the meaning of this function based on whether the version is external or internal.
We only want specific commits to link to a commit, not the version itself. We need another function to add a specific URL to the commit I think.
| commit=self.commit | ||
| ) | ||
|
|
||
| return '' |
There was a problem hiding this comment.
I do wonder if we should return None here, instead of ''. I don't think it matters too much though.
There was a problem hiding this comment.
for URL's I think its better to return empty string. as it will show href='' not href=None in the template.
There was a problem hiding this comment.
We should not output the link if we don't have an href attribute.
There was a problem hiding this comment.
Merging this, but we should fix it ^^
There was a problem hiding this comment.
@ericholscher Updated please see this 763b857
TODO: