Skip to content

added a profile update log #563

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

Open
WallerTrevor wants to merge 1 commit into
base: master
Choose a base branch
from
Open

added a profile update log #563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions app/controllers/profiles_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def show
@posts = @user.posts.select_overview_columns.public?.order("#{ allowed_sort_params.include?(params[:sort_posts]) ? params[:sort_posts] : "created_at" } DESC").page(params[:page])
@blocks = Block.where(user_id: @user.id, content_type: :profile).order(position: :asc, created_at: :asc)
@collections = @user.collections.includes(:posts).where("posts_count > ?", 0)
@updates = Revision.where(post_id: @user.posts.pluck(:id)).order(created_at: :desc).limit(50)

@Mitcheljager Mitcheljager Feb 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This has a few issues:

  • This will include posts that are private or in draft
  • This will load posts with n+1 because each revision will need to query it's post. It's not an n+1 query, but it is still quite inefficient
  • A limit of 50 is really quite high, that's a lot of revisions to load at once. Better might be to limit it to like 20 at a time with pagination

@Mitcheljager Mitcheljager Mar 1, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

One thing I forgot to mention earlier, this will slow down all profiles even if the feed is never shown. This would be better to load async via a partial just like how tabs are loaded on post pages.


respond_to do |format|
format.html
Expand Down
88 changes: 88 additions & 0 deletions app/views/blocks/profile/_update_log.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<%# app/views/blocks/profile/_update_log.html.erb %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We know what file we're in, we really don't need that at the top

<div class="mt-1/1">

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Other profile tabs have a title at the top, it would be nice if this had that too

<% if @updates&.any? %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@updates can't be nil since .where will fall back to an empty collection if nothing is found, so the optional chaining here is irrelevant

<%# Removed the 'standout' and 'p-0' classes to remove the box/border %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This comment is odd, why do you want me to remove these classes?

<div class="feed">
<% @updates.each_with_index do |revision, index| %>
<%# Use 'feed__item' to ensure the site's global CSS handles the horizontal layout %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ok, that's the case for all classes. We don't need to state that we're using a class, that's redundant.

<div class="feed__item <%= "border-top" unless index == 0 %> py-1/2">

<%# Left Column: Thumbnail and Code %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What is this comment supposed to indicate?

<div class="feed__info">
<%= render "posts/thumbnail", post: revision.post %>
<%= render "posts/code", post: revision.post, item_class: "code feed__code" %>

<p class="feed__author">
by <%= link_to revision.post.user.clean_username, user_profile_path(revision.post.user) %>
</p>

<p class="feed__date">
<%= time_ago_in_words(revision.created_at) %> ago
</p>
</div>

<%# Right Column: Text Content (Left-Aligned by default in 'feed__item') %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm not sure what this comment means

<div class="feed__content">
<h3 class="feed__title">
<%= link_to revision.post.title, post_path(revision.post.code) %>
</h3>

<% if revision.version.present? %>
<p class="feed__version">v<%= revision.version %></p>
<% end %>

<% if revision.description.blank? %>
<p class="mb-0 text-muted italic">
No update notes provided.
</p>
<% else %>
<h5 class="feed__update-notes mt-1/2 mb-1/8">Update notes</h5>
<div class="revision-description" data-role="expandable-content">
<%# 'item__description' ensures the markdown text aligns left %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No it doesn't? Not sure what this comment means

<div class="item__description mt-1/8 line-clamp-3" data-target="expandable-text">
<%= sanitized_markdown(revision.description) %>
</div>

<% if revision.description.length > 200 %>
<button class="text-primary text-xs font-bold mt-1/4 p-0 bg-transparent border-0 cursor-pointer hover-underline"

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

For complex classlists like this you might prefer to use a defined class, something like button button--link. This is very tailwind-y and unreadable. Not something we haven't done before, but something we should avoid either way

data-action="toggle-expand">
Read more
</button>
<% end %>
</div>
Comment on lines +40 to +52

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Did you check if this whole expandable-content and data-action=toggle-expand works? It's not something that exists but I don't see any new javascript to make this work.

This wouldn't work well either way. Cutting off the text with a line-clamp might be problematic if the feed contains things like images or titles and such. You might prefer to cut it off at a certain height instead of a certain line count. Or perhaps just not at all.

<% end %>
</div>
</div>
<% end %>
</div>
Comment on lines +7 to +57

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This is largely a copy of _feed_item.html.erb. Rather than copy pasting the same code, you can reuse that partial with <%= render "feed/feed_item", revision: revision %>. That way you prevent having to make changes in multiple places. Everything re-using the same file.

<% else %>
<%# Simple centered message if empty, but without the 'standout' box %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Comments describing something that isn't there make little sense.

<div class="py-2 text-center">
<p class="m-0 text-muted">No updates found for this user's gamemodes.</p>
</div>
<% end %>
</div>

<%# Keep your Styles and Scripts at the bottom as before %>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No, don't keep it here. Javascript and css go into their own respective locations and files.

<style>
.line-clamp-3 {
display: -webkit-box;
-webkit-line-clamp: 3;
-webkit-box-orient: vertical;
overflow: hidden;
}
.expanded .line-clamp-3 {
display: block;
-webkit-line-clamp: unset;
}
</style>

<script>
document.querySelectorAll('[data-action="toggle-expand"]').forEach(button => {
button.addEventListener('click', (e) => {

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This event won't get cleaned up when navigating which is problematic with Turbolinks. If you navigate away from this page the event listeners will stay in memory. If you navigate back to this page these event listeners will be attached again over top the other ones. Go back and forth a few times and each button will fire many times over, making it impossible to toggle the content here.

const container = button.closest('[data-role="expandable-content"]');
container.classList.toggle('expanded');
button.textContent = container.classList.contains('expanded') ? 'Read less' : 'Read more';
});
});
</script>
Comment on lines +80 to +88

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I now see you put the javascript here. Javascript typically goes into its own file in app/javascript/src. That way it's re-usable, in a predictable location, and you can use typescript, which you can't do here.

7 changes: 6 additions & 1 deletion app/views/profiles/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<%= link_to "Highlights", "#", class: "tabs__item #{"tabs__item--active" unless params[:tab].present? && params[:tab] != "highlights"}", data: { action: "set-tab", target: "highlights" } if @blocks.any? %>
<%= link_to "Codes", "#", class: "tabs__item #{ "tabs__item--active" unless (params[:tab].blank? && @blocks.any?) || (params[:tab].present? && params[:tab] != "codes") }", data: { action: "set-tab", target: "codes" } %>
<%= link_to "Collections", "#", class: "tabs__item #{"tabs__item--active" if params[:tab] == "collections"}", data: { action: "set-tab", target: "collections" } %>
<%= link_to "Update Log", "#", class: "tabs__item #{"tabs__item--active" if params[:tab] == "update-log"}", data: { action: "set-tab", target: "update-log" } %>
</nav>
</div>
</div>
Expand All @@ -38,7 +39,11 @@
<%= render "blocks/profile/list" %>
</div>

<div class="tabs-content <%= "tabs-content--active" if params[:tab] == "update-log" %>" data-tab="update-log">
<%= render "blocks/profile/update_log" %>
</div>

<div class="tabs-content <%= "tabs-content--active" if params[:tab] == "collections" %>" data-tab="collections">
<%= render "blocks/profile/user_collections" %>
</div>
</div>
</div>

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This has a trailing space but no newline at the end of the file

70 changes: 1 addition & 69 deletions package-lock.json
View file
Open in desktop

@Mitcheljager Mitcheljager Feb 28, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please don't include changes in package-lock, these seem to be accidental

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading