gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Changelog page that pulls from Github Release notes #867

Merged
mattia-malnis merged 3 commits from feature/changelog-page into main 2024-06-15 04:40:50 +08:00
Conversation 8 Commits 3 Files Changed 6 +100 -26
mattia-malnis commented 2024-06-12 23:26:32 +08:00 (Migrated from github.com)
Copy Link

Closes #831

https://github.com/maybe-finance/maybe/assets/15941778/153a4235-357b-4f81-9801-639d2d736131

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.

immagine
Closes #831 https://github.com/maybe-finance/maybe/assets/15941778/153a4235-357b-4f81-9801-639d2d736131 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. <img width="584" alt="immagine" src="https://github.com/maybe-finance/maybe/assets/15941778/ddaa1edd-ac5b-47bd-995c-07cc22621f08">
zachgoll (Migrated from github.com) reviewed 2024-06-13 00:22:24 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

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.

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.
app/assets/stylesheets/application.tailwind.css
zachgoll (Migrated from github.com) commented 2024-06-13 00:19:50 +08:00
Copy Link

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!

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: ```erb <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](https://github.com/tailwindlabs/tailwindcss-typography?tab=readme-ov-file#customizing-the-css): ```js // 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": ```css .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!
app/views/pages/changelog.html.erb
@@ -2,11 +2,27 @@
<%= render "settings/nav" %>
<% end %>
<div class="space-y-4">
zachgoll (Migrated from github.com) commented 2024-06-13 00:21:12 +08:00
Copy Link

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

Small nitpick here, but we should probably change to an `h2` to avoid duplicate H1s on the page.
zachgoll (Migrated from github.com) reviewed 2024-06-13 01:11:17 +08:00
app/assets/stylesheets/application.tailwind.css
zachgoll (Migrated from github.com) commented 2024-06-13 01:11:17 +08:00
Copy Link

@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.

@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.
flacnut commented 2024-06-13 01:52:03 +08:00 (Migrated from github.com)
Copy Link

Thanks for doing this!

Thanks for doing this!
🚀 1
mattia-malnis commented 2024-06-14 03:07:47 +08:00 (Migrated from github.com)
Copy Link

@zachgoll thanks for the feedback! I think I can finish it tomorrow!

@zachgoll thanks for the feedback! I think I can finish it tomorrow!
🚀 1
mattia-malnis (Migrated from github.com) reviewed 2024-06-14 15:44:04 +08:00
app/assets/stylesheets/application.tailwind.css
mattia-malnis (Migrated from github.com) commented 2024-06-14 15:44:04 +08:00
Copy Link

@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

@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
mattia-malnis (Migrated from github.com) reviewed 2024-06-14 15:46:12 +08:00
app/views/pages/changelog.html.erb
@@ -2,11 +2,27 @@
<%= render "settings/nav" %>
<% end %>
<div class="space-y-4">
mattia-malnis (Migrated from github.com) commented 2024-06-14 15:46:11 +08:00
Copy Link

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

I've changed it to an h2 to avoid duplicate H1s on the page!
zachgoll (Migrated from github.com) approved these changes 2024-06-14 21:48:26 +08:00
zachgoll (Migrated from github.com) left a comment
Copy Link

Awesome work, this looks great!

Awesome work, this looks great!
app/views/pages/changelog.html.erb
@@ -2,11 +2,27 @@
<%= render "settings/nav" %>
<% end %>
<div class="space-y-4">
zachgoll (Migrated from github.com) commented 2024-06-14 21:48:04 +08:00
Copy Link

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

I know other spots in the codebase are still lacking this, but we should probably include an i18n `t(".title")` here
mattia-malnis (Migrated from github.com) reviewed 2024-06-15 01:06:11 +08:00
app/views/pages/changelog.html.erb
@@ -2,11 +2,27 @@
<%= render "settings/nav" %>
<% end %>
<div class="space-y-4">
mattia-malnis (Migrated from github.com) commented 2024-06-15 01:06:11 +08:00
Copy Link

Sure, I updated it!

Sure, I updated it!
Sign in to join this conversation.
Reviewers
No Reviewers
zachgoll
mattia-malnis
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#867
Delete Branch "feature/changelog-page"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?