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

Jump to bottom

Changelog page that pulls from Github Release notes #867

Merged
merged 3 commits into from
Jun 14, 2024
Merged

Changelog page that pulls from Github Release notes #867

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d6e3f2
Changelog page that pulls from Github Release notes
mattia-malnis Jun 12, 2024
f245ee3
Review changelog page styles
mattia-malnis Jun 14, 2024
cf3f216
Move changelog page title to i18n translations
mattia-malnis Jun 14, 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
37 changes: 37 additions & 0 deletions app/assets/stylesheets/application.tailwind.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,43 @@
@apply after:content-[''] after:block after:absolute after:top-0.5 after:left-0.5 after:bg-white after:w-4 after:h-4 after:rounded-full after:transition-transform after:duration-300 after:ease-in-out;
@apply peer-checked:bg-green-600 peer-checked:after:translate-x-4;
}

.releases-notes-row {
@apply mb-12;
}
.releases-notes-details {
@apply text-gray-500 text-sm;
}
.releases-notes-details h2 {
@apply text-lg mt-3.5 mb-2.5;
}
.releases-notes-details ul {
@apply list-disc my-2.5 pl-4;
}
.releases-notes-details p {
@apply my-2.5;
}
.releases-notes-details a {
@apply underline;
}
.releases-notes-details .user-mention {
@apply font-bold no-underline;
}
.releases-notes-details details {
@apply my-3.5 rounded-xl;
}
.releases-notes-details details video {
@apply rounded-b-xl;
}
.releases-notes-details summary {
@apply flex items-center gap-x-1;
}
.releases-notes-details .dropdown-caret {
@apply content-none border-4 border-b-0 border-transparent border-t-gray-500 size-0 inline-block;
}
.releases-notes-details .octicon {
@apply inline-block overflow-visible align-text-bottom fill-current;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Definitely a tricky one here. I think this is a good opportunity for us to evaluate the usage of typography within the app.

Ideally, I think what we should probably consider here is an API like so:

<div class="prose prose--github-release-notes">
  <%= release_notes[:body].html_safe %>
</div>

We have prose as the "base" styling mechanism, and then prose--github-release-notes as a "specialization" where we can add styles for things like .user-mention, .octicon, and .dropdown-caret which don't exist anywhere else.

That way, in tailwind.config.js, we can do all of our "base" prose overrides:

// SAMPLE, not actual styles

/** @type {import('tailwindcss').Config} */
module.exports = {
  theme: {
    extend: {
      typography: {
        DEFAULT: {
          css: {
            color: '#333',
            a: {
              color: '#3182ce',
              '&:hover': {
                color: '#2c5282',
              },
            },
          },
        },
      },
    },
  },
  plugins: [
    require('@tailwindcss/typography'),
    // ...
  ],
}

And then in our global Tailwind CSS file, we can apply the "specializations":

.prose--github-release-notes {
  .octicon {
    @apply inline-block overflow-visible align-text-bottom fill-current;
  }

  .dropdown-caret {
    @apply content-none border-4 border-b-0 border-transparent border-t-gray-500 size-0 inline-block;
  }
}

So in summary, the changes required here would be:

  1. Delete existing .prose class in Tailwind CSS file (it's not being used anywhere)
  2. Add .prose--github-release-notes and all the "specializations" to the Tailwind CSS file
  3. Apply all "base" prose overrides directly in tailwind.config.js
  4. Move all non-typography styles directly to the ERB files (e.g. the .release-notes-row, etc.)

Let me know what you think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mattia-malnis for those "base" prose styles for the Tailwind Config file, I worked with @justinfar to standardize this a bit and included it in the updated design file:

https://www.figma.com/design/lonJmVk3HYkwZoIO7xYP2w/Maybe-App-(Community)?node-id=3789-202&t=UY4qtUbF3hRvnpp1-0

Please note—this is an example that we should try to be as "generic" as possible with when applying to those "base" styles in the Tailwind config. These "base" styles should work with any HTML we need to render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachgoll your solution is definitely better, I didn't know about this possibility with Tailwind! I reviewed the code, let me know if everything is ok

}

/* Small, single purpose classes that should take precedence over other styles */
Expand Down
1 change: 1 addition & 0 deletions app/controllers/pages_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ def dashboard
end

def changelog
@releases_notes = Provider::Github.new.fetch_latest_releases_notes
end

def feedback
Expand Down
20 changes: 20 additions & 0 deletions app/models/provider/github.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,26 @@ def fetch_latest_upgrade_candidates
end
end

def fetch_latest_releases_notes
begin
Rails.cache.fetch("latest_github_releases_notes", expires_in: 2.hours) do
releases = Octokit.releases(repo)
releases.map do |release|
{
avatar: release.author.avatar_url,
name: release.name,
published_at: release.published_at,
body: Octokit.markdown(release.body, mode: "gfm", context: repo)
}
end
end

rescue => e
Rails.logger.error "Failed to fetch latest GitHub releases notes: #{e.message}"
[]
end
end

private
def repo
"#{owner}/#{name}"
Expand Down
22 changes: 19 additions & 3 deletions app/views/pages/changelog.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,25 @@
<div class="space-y-4">
<h1 class="text-gray-900 text-xl font-medium mb-4">What's New</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know other spots in the codebase are still lacking this, but we should probably include an i18n t(".title") here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I updated it!

<div class="bg-white shadow-xs border border-alpha-black-25 rounded-xl p-4">
<div class="flex justify-center items-center py-20">
<p class="text-gray-500">Changelog coming soon...</p>
</div>
<% @releases_notes.each do |release_notes| %>
<div class="flex justify-between gap-4 releases-notes-row">
<div class="w-1/3">
<div class="px-3 flex items-center gap-3">
<div class="text-white shrink-0 w-9 h-9">
<%= image_tag release_notes[:avatar], class: "rounded-full w-full h-full object-cover" %>
</div>
<div>
<div class="text-gray-900 font-medium text-sm"><%= release_notes[:name] %></div>
<div class="text-gray-500 text-sm"><%= release_notes[:published_at].strftime("%B %d, %Y") %></div>
</div>
</div>
</div>
<div class="w-2/3 releases-notes-details">
<h1 class="mb-5 text-xl text-gray-900"><%= release_notes[:name] %></h1>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nitpick here, but we should probably change to an h2 to avoid duplicate H1s on the page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to an h2 to avoid duplicate H1s on the page!

<%= release_notes[:body].html_safe %>
</div>
</div>
<% end %>
</div>
<div class="flex justify-between gap-4">
<%= previous_setting("Imports", imports_path) %>
Expand Down