Skip to content

Comments

Action Mailer Guide#8

Open
bhumi1102 wants to merge 45 commits intomainfrom
bhumi-guides-action-mailer
Open

Action Mailer Guide#8
bhumi1102 wants to merge 45 commits intomainfrom
bhumi-guides-action-mailer

Conversation

@bhumi1102
Copy link
Owner

@bhumi1102 bhumi1102 commented May 31, 2024

Action Mailer Guide is ready for internal review. In addition to editing, good amount of structural changes here and bit of new content (there is one thing I'm still thinking over - the table in the configuration section.)

  • Mailers now actually inherit from a generated ApplicationMailer, which in turn inherit from ActionMailer::Base.
  • The output to generating a mailer might be different depending on whether the base ApplicationMailer and layout have already been generated, which will already happen when creating a new app, or if they're not there the first time the mailer is generated.
  • Maybe the generator should be shown by passing an action name directly? (or as a second example or subsection?) bin/rails generate mailer User welcome_email
  • Complete List of Action Mailer should be linked. (in fact, maybe we don't even "need" this to be a section, but rather part of the text when introducing the other mailer methods? And then a full section on Attachments)
  • The HTML view has a complete layout with DOCTYPE/head/body, but it should be using the application mailer's layout, so that example will likely double the HTML, it is incorrect.
  • The guide shows how to build a multipart email (HTML + Text), but doesn't really explain why, what are the benefits, if it's really needed, etc., might be a worth addition to consider. There's a section later in the guide that's about "multipart emails", that we could link to from the top, and perhaps expand a bit on that.
  • I wonder if we should instead show an example of sending the email via console, after we have the mailer and associated model? And then proceed with showing the controller version.
  • Link to Active Job guide, there's a mention when explaining how to call the mailer, and a Note. That note should likely be simplified and linked instead. (less duplicated content, no need to mention Sidekiq,etc. elsewhere, etc.)
  • If we do show how to use via console, it'd be a good introduction to start with deliver_now, and then explain deliver_later + Active Job afterwards.
  • Auto Encoding Header Values seems like a topic/section that could be pushed down, it's not a common need.
    Decided to remove as it is an extraneous detail.
  • Attachments could be a larger subsection, right now it's a 3rd level section that doesn't show in the sections list on the right.
  • "Action Mailer 3.0" no need to mention older versions any longer.
  • Under the Layouts section, it doesn't mention where the layout should be located. (app/views/layouts), and it should probably mention the default mailer layout now too. (see other comment about about the email template containing the full HTML tags)
  • There's a section about "Dynamic Delivery Options" that shows how to pass SMTP & etc. configs, but the configuration section is later in the guide, this could be reorganized to better flow, or at least linked.
  • There's some information about views, layouts, rendering without a template, etc., scattered, maybe it makes sense to group these related pieces a little bit better.
  • Under the configuration section, deliveries is not really a configuration but something to use in dev/test., not sure it's where it should be mentioned.
  • A new section about rescuing errors could be added too. Here's the API for reference.
  • No mention of I18n, maybe could have at very brief section or paragraph/note somewhere that points to the I18n guide on the matter.

@bhumi1102 bhumi1102 changed the title Action Mailer Guide Action Mailer Guide [WIP] Jun 3, 2024
@bhumi1102 bhumi1102 changed the title Action Mailer Guide [WIP] Action Mailer Guide Sep 3, 2024
Copy link
Collaborator

@p8 p8 left a comment

Choose a reason for hiding this comment

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

This was a nice read @bhumi1102 ! I've added a bunch of suggestions.

Copy link
Collaborator

@OughtPuts OughtPuts left a comment

Choose a reason for hiding this comment

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

Great job @bhumi1102, really well-structured and clear :)

@OughtPuts
Copy link
Collaborator

Actually one other thing I was going to add was - we seem to refer to multipart emails quite a few times - I wonder if it's worth double checking each instance back to back for any repetition or way we can make that a bit more concise? May not be, but it was something that stood out to me when reading!

Copy link
Collaborator

@Ridhwana Ridhwana left a comment

Choose a reason for hiding this comment

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

This is great @bhumi1102 ✨ Left some small suggestions.

bhumi1102 and others added 15 commits September 10, 2024 14:37
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Harriet Oughton <73295547+OughtPuts@users.noreply.github.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Harriet Oughton <73295547+OughtPuts@users.noreply.github.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Harriet Oughton <73295547+OughtPuts@users.noreply.github.com>
Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
Co-authored-by: Petrik de Heus <petrik@deheus.net>
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.

4 participants