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

Show message if version list truncated #6276

Merged
merged 3 commits into from Feb 19, 2020

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Oct 10, 2019

Show a message if we are showing the max inactive versions on the version list screen.

Technically, if the project has exactly 100 versions the message is shown even if nothing was truncated but I think that is acceptable in this instance.

Fixes #6265

@davidfischer davidfischer requested a review from Oct 10, 2019
stsewd
stsewd approved these changes Oct 10, 2019
readthedocs/templates/projects/project_version_list.html Outdated Show resolved Hide resolved
@@ -367,7 +369,7 @@ def project_versions(request, project_slug):
version_filter = request.GET.get('version_filter', '')
if version_filter:
inactive_versions = inactive_versions.filter(verbose_name__icontains=version_filter)
inactive_versions = inactive_versions[:100]
inactive_versions = inactive_versions[:max_inactive_versions]
Copy link
Member

@ericholscher ericholscher Oct 14, 2019

Choose a reason for hiding this comment

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

Feels like the length check should be done here, before we truncate it. Otherwise won't the template never show additional versions because it's already cut off at max_inactive_versions total?

Copy link
Contributor Author

@davidfischer davidfischer Nov 7, 2019

Choose a reason for hiding this comment

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

The message is shown because there will be 100 inactive versions and the check checks >=. It is an extra query to do a count but I don't think it'll be too costly.

Copy link
Contributor

@agjohnson agjohnson left a comment

This feels a little like we're adding directions explaining our UI rather than fixing the UI issues. I do understand this is a transitional UI, but don't want to continue investing time like this into this particular UI.

More future proof fixes to this problem will be live filtering on the results and moving the inactive version list into a separate UI and view where it won't be confused with the active version list. Some of these changes are outlined in #6238

I'm probably -0 on the underlying issue, but we've already spent time on this so I think the best we can do is to give the user more context.

{% if inactive_versions|length >= max_inactive_versions %}
<p>
{% blocktrans trimmed %}
Only the first {{ max_inactive_versions }} inactive versions were shown. Please <a href="#version-filter">filter the list</a>.
Copy link
Contributor

@agjohnson agjohnson Oct 14, 2019

Choose a reason for hiding this comment

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

We can give the user more context and avoid having to directly tell them how to use our UI. A couple of options:

  • "Showing first {{ max_inactive_versions }} of {{ number of matches }}"
  • "X matching inactive versions not shown"

Copy link
Contributor Author

@davidfischer davidfischer Nov 7, 2019

Choose a reason for hiding this comment

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

This will involve an extra query as I outlined #6276 (comment) but I suppose the cost isn't too high. I can add it.

@humitos humitos requested a review from agjohnson Nov 18, 2019
@agjohnson agjohnson added the PR: work in progress label Nov 26, 2019
@stale
Copy link

@stale stale bot commented Jan 2, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Jan 2, 2020
@stsewd stsewd removed the Status: stale label Jan 6, 2020
@stale
Copy link

@stale stale bot commented Feb 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale label Feb 16, 2020
Copy link
Member

@ericholscher ericholscher left a comment

Having this is better than not having it, so I'm 👍 on shipping it.

@stale stale bot removed the Status: stale label Feb 17, 2020
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Feb 17, 2020

I think a count would be more helpful here still. I'm not worried about the cost of the extra query. I'm fine waiting until this change is in.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 18, 2020

I just forgot about this one. I'm adding the extra query.

- Only show if the number exceeds the displayed number
@davidfischer davidfischer removed the PR: work in progress label Feb 18, 2020
@humitos humitos merged commit 098db3d into master Feb 19, 2020
3 checks passed
@humitos humitos deleted the davidfischer/version-truncated-message branch Feb 19, 2020
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.

6 participants