-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Changelog page that pulls from Github Release notes #867
Changelog page that pulls from Github Release notes #867
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for tackling this!
Visually it all looks good. I left some suggestions around how we can consolidate some of the "prose" styles and leverage the Tailwind typography plugin a bit.
} | ||
.releases-notes-details .octicon { | ||
@apply inline-block overflow-visible align-text-bottom fill-current; | ||
} |
There was a problem hiding this comment.
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:
- Delete existing
.prose
class in Tailwind CSS file (it's not being used anywhere) - Add
.prose--github-release-notes
and all the "specializations" to the Tailwind CSS file - Apply all "base" prose overrides directly in
tailwind.config.js
- Move all non-typography styles directly to the ERB files (e.g. the
.release-notes-row
, etc.)
Let me know what you think!
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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
app/views/pages/changelog.html.erb
Outdated
</div> | ||
</div> | ||
<div class="w-2/3 releases-notes-details"> | ||
<h1 class="mb-5 text-xl text-gray-900"><%= release_notes[:name] %></h1> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Thanks for doing this! |
@zachgoll thanks for the feedback! I think I can finish it tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, this looks great!
app/views/pages/changelog.html.erb
Outdated
@@ -4,9 +4,25 @@ | |||
<div class="space-y-4"> | |||
<h1 class="text-gray-900 text-xl font-medium mb-4">What's New</h1> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I updated it!
* Changelog page that pulls from Github Release notes * Review changelog page styles * Move changelog page title to i18n translations
* feat: make transaction container fixed height * feat: pagination per page query * fix: linting errors * Changelog page that pulls from Github Release notes (#867) * Changelog page that pulls from Github Release notes * Review changelog page styles * Move changelog page title to i18n translations * Bump to 0.1.0-alpha.6 Signed-off-by: Zach Gollwitzer <[email protected]> * Bump aws-sdk-s3 from 1.152.0 to 1.152.3 (#880) Bumps [aws-sdk-s3](https://github.com/aws/aws-sdk-ruby) from 1.152.0 to 1.152.3. - [Release notes](https://github.com/aws/aws-sdk-ruby/releases) - [Changelog](https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-s3/CHANGELOG.md) - [Commits](https://github.com/aws/aws-sdk-ruby/commits) --- updated-dependencies: - dependency-name: aws-sdk-s3 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump mocha from 2.3.0 to 2.4.0 (#878) Bumps [mocha](https://github.com/freerange/mocha) from 2.3.0 to 2.4.0. - [Changelog](https://github.com/freerange/mocha/blob/main/RELEASE.md) - [Commits](freerange/mocha@v2.3.0...v2.4.0) --- updated-dependencies: - dependency-name: mocha dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump octokit from 8.1.0 to 9.1.0 (#877) Bumps [octokit](https://github.com/octokit/octokit.rb) from 8.1.0 to 9.1.0. - [Release notes](https://github.com/octokit/octokit.rb/releases) - [Changelog](https://github.com/octokit/octokit.rb/blob/main/RELEASE.md) - [Commits](octokit/octokit.rb@v8.1.0...v9.1.0) --- updated-dependencies: - dependency-name: octokit dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump rails from `f9c847f` to `5d34172` (#879) Bumps [rails](https://github.com/rails/rails) from `f9c847f` to `5d34172`. - [Release notes](https://github.com/rails/rails/releases) - [Commits](rails/rails@f9c847f...5d34172) --- updated-dependencies: - dependency-name: rails dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Zach Gollwitzer <[email protected]> * Update issue templates * Add merchant select when editing transaction (#885) * Transaction transfers, payments, and matching (#883) * Add transfer model and clean up family snapshot fixtures * Ignore transfers in income and expense snapshots * Add transfer validations * Implement basic transfer matching UI * Fix merge conflicts * Add missing translations * Tweak selection states for transfer types * Add missing i18n translation * Ensure correct form's hidden input for selectedIds (#891) * feat: make transaction container fixed height * feat: pagination per page query * fix: linting errors * Transaction transfers, payments, and matching (#883) * Add transfer model and clean up family snapshot fixtures * Ignore transfers in income and expense snapshots * Add transfer validations * Implement basic transfer matching UI * Fix merge conflicts * Add missing translations * Tweak selection states for transfer types * Add missing i18n translation * feat: make transaction container fixed height * feat: pagination per page query * fix: linting errors * revert unnecessary changes * revert unnecessary changes * code review changes * code review changes * code review changes * remove unused imports * fix: unit tests * remove border * fix: transaction padding * fix: transaction container height --------- Signed-off-by: Zach Gollwitzer <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Karan Kiri <[email protected]> Co-authored-by: Mattia <[email protected]> Co-authored-by: Zach Gollwitzer <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Zach Gollwitzer <[email protected]> Co-authored-by: Jakub Kottnauer <[email protected]> Co-authored-by: ziraq young <[email protected]>
Closes #831
Registrazione.schermo.2024-06-12.alle.17.09.33.mov
I'm not 100% sure that I wrote the CSS correctly using Tailwind. Any suggestions are welcome!
I also think it could be a good idea to rename "changelog" to "what's new" in the menu that opens when clicking the avatar image.