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

added post count to user posts page #1108

Merged
merged 3 commits into from Jul 10, 2023

Conversation

cellio
Copy link
Member

@cellio cellio commented Jul 9, 2023

Adds the total number of posts to the user profile's posts page, like on category lists and search results. Addresses https://meta.codidact.com/posts/287868.

With my change:

screenshot

Before:
screenshot

(Second try, without the git noise I hope.)

Copy link
Contributor

@trichoplax trichoplax left a comment

Choose a reason for hiding this comment

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

Looks good. I've requested 2 minor changes.

app/views/users/posts.html.erb Outdated Show resolved Hide resolved
Comment on lines 9 to 15
<div>
<% post_count = @posts.count %>
<div class="has-color-tertiary-500 category-meta" title="<%= post_count %>">
<div>
<%= short_number_to_human post_count, precision: 1, significant: false %>
<%= 'post'.pluralize(post_count) %> &middot;
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

The opening <div> on line 9 is redundant (it doesn't have a corresponding closing </div>, and doesn't need to be there). It's causing layout problems when adjusting the window width, so at some widths the list of posts jumps up onto the same line as the sorting buttons. Removing line 9 and adjusting the indentation of the following lines should make it clearer which opening <div> matches up with which closing </div>.

The root cause behind this section of code being difficult to reason about is the line <div class="has-color-tertiary-500 category-meta" title="<%= post_count %>">. This div has a title of "post_count", which makes it look like it should be containing only the post count, but it actually contains the sorting buttons too. So its closing </div> is further down the code than expected.

It looks like the code has been based on the similar code in app/views/categories/show.html.erb, which has the same confusing "post_count" on a div that contains more than just the post count. This is also the root cause behind Title text (tooltip/hovertext) shows on unrelated parts of the page. Fixing that bug will make this section of code much easier to read. I recommend against trying to fix it in this pull request - I prefer to keep pull requests focused. Instead I've added an issue (#1109) for that Meta post and mentioned that the fix should be applied to both the category level posts list and the user posts list.

I'm happy with introducing a very minor bug with this pull request, as it's still an overall improvement and the bug is recorded.

Suggested change
<div>
<% post_count = @posts.count %>
<div class="has-color-tertiary-500 category-meta" title="<%= post_count %>">
<div>
<%= short_number_to_human post_count, precision: 1, significant: false %>
<%= 'post'.pluralize(post_count) %> &middot;
</div>
<% post_count = @posts.count %>
<div class="has-color-tertiary-500 category-meta" title="<%= post_count %>">
<div>
<%= short_number_to_human post_count, precision: 1, significant: false %>
<%= 'post'.pluralize(post_count) %> &middot;
</div>

Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to mention that the middot (&middot;) used to separate the post count from other buttons is redundant here (since there are no other buttons). I won't add a suggested change yet as I don't want this trivial change to mess up the larger suggestion it would clash with.

Once the larger change is applied (if you're happy with it) I'm happy to make a separate suggestion for the middot, or you could just make a separate commit - whatever's easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to edit for the stray div anyway, so I got rid of the &middot; while I was there.

I agree that we want a general fix for the misplaced tooltip text and should do it separately.

cellio and others added 2 commits July 10, 2023 15:29
@cellio cellio merged commit 6dae59b into develop Jul 10, 2023
3 of 4 checks passed
@cellio cellio deleted the cellio/post-count-on-user-profile-2 branch July 10, 2023 21:24
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.

None yet

2 participants