-
Notifications
You must be signed in to change notification settings - Fork 12
Action text rhino #570
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
Action text rhino #570
Conversation
maebeale
left a comment
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.
Rhino was SUCH a great find, @jmilljr24 !
And this is a critical feature. Well done!!!
| class RichTextMigrator | ||
| include ActionText::ContentHelper | ||
|
|
||
| PLACEHOLDER_TEXT = "image not found" |
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 think we should add some brackets here so it sticks out more. Maybe [image not found] ?
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.
For sure! I should be able to add the file name or some way to identify it with brackets.
| @@ -0,0 +1,101 @@ | |||
| <div class="min-h-screen py-8"> | |||
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.
Yeah I think this is the best way -- just a full copy of show but calling rhino_text.
config/initializers/action_text.rb
Outdated
| @@ -0,0 +1,5 @@ | |||
| default_allowed_tags = Class.new.include(ActionText::ContentHelper).new.sanitizer_allowed_tags | |||
| ActionText::ContentHelper.allowed_tags = default_allowed_tags.merge(%w[iframe table colgroup col tbody tr th td]) | |||
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 think we should also whitelist thead and tfoot here, if they're not already in default allowed tags
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.
Thanks! I don't know the last time I built a table. Good catch
What is the goal of this PR and why is this important?
We need a new rich text editor as ckeditor v4 was EOL.
How did you approach the change?
Add ActionText to keep aligned with rails. Add Rhino-Editor. This handles attachments etc. with ActionText compatibility based off trix. The editor itself used Tiptap under the hood and is easily expandable with extensions.
Looking at the current ckeditor data being used, the following main features are needed an are included in this PR:
Anything else to add?
Currently added to
Resource. This needs to be added to all other rich text fields. Old data can be moved to ActionText via a migrator.RichTextMigrator.new(record, :text).migrate!View the rendered content
/resources/:id/show_testNew
has_rich_textassociations should following the naming pattern of"rhino_#{old_column}"for examplehas_rich_text :rhino_text