-
Notifications
You must be signed in to change notification settings - Fork 147
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
Introduce Message Formatting with Actiontext Trix Editor #1013
Conversation
two questions that are open to me:
|
@lentschi could you have another look at this? :) It' the first rake task I ever wrote, but I think it works as intended. For the rake task I'd leave a note addressing the new config field in the Release Notes ... |
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.
Quick pass on this, LGTM! Thanks for working on it, definitely nice feature to have.
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 don't know what you preferred way of working is, but if
a) you want to squash you changes during merge, the PR contains IMHO too much unrelated changes (one commit should change only as little as possible. e.g. (1) just add the trix editor, (2) adopt messaging to use trix, (3) add prune_old_attachments
)
b) you want to merge it with the whole commit history, the PR contains too much useless commits like typo fixes.
since developers read code much more often than they write it, creating a clean git history should have some importance. (especially if nobody writes any kind of code documentation ;-))
db/migrate/20230215085312_migrate_message_body_to_action_text.rb
Outdated
Show resolved
Hide resolved
db/migrate/20230215085312_migrate_message_body_to_action_text.rb
Outdated
Show resolved
Hide resolved
thanks for your review paroga! |
c367681
to
580b2b8
Compare
580b2b8
to
4b37cbf
Compare
This introduces to possibility to send formatted messages and attachments with the messages plugin.
*
[ ] test email reply with attachment, is it shown in foodsoft then?tried around a bit but still not sure what's going on there, I'd guess the message data will just be shown as a plaintext.
I think if this is a use case we can have a look at some other point ...