Skip to content
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

Add note regarding "trix-content" class. [ci-skip] #41889

Merged
merged 1 commit into from May 30, 2021

Conversation

stevepolitodesign
Copy link
Contributor

Summary

Passing custom classes into the rich_text_area form helper removes the trix-content class. This class is needed for default styling. This can lead developers to think their styles are broken, when in fact all that is missing is the correct class.

Although this is documented in the Trix README, it's not highlighted in the official Rails Guides.

Example

<%= form.rich_text_area :content %> will render <trix-editor id="content" input="trix_input_post_1" class="trix-content" ...></trix-editor>

<%= form.rich_text_area :content, class: "foo" %> will render <trix-editor id="content" input="trix_input_post_1" class="foo" ...></trix-editor>. Note how the trix-content no longer renders.

Other Information

Related Tweet

@rails-bot rails-bot bot added the docs label Apr 9, 2021
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

I think documenting this is a great idea. 👍

Does it make sense to extend the API documentation as well? Right now, it documents that the parameter exists, but it does not explicitly point out that passing an argument will change the behavior.

guides/source/action_text_overview.md Outdated Show resolved Hide resolved
@rails-bot rails-bot bot added the actiontext label Apr 10, 2021
actiontext/app/helpers/action_text/tag_helper.rb Outdated Show resolved Hide resolved
@@ -97,6 +97,8 @@ Then refer to this field in the form for the model:
<% end %>
```

**Note:** The `rich_text_area` form helper automatically adds the `trix-content` CSS class to the Trix editor element. `trix-content` is responsible for the editor's [default styling](https://github.com/rails/rails/blob/adc5eb66f861a98da32f7c682155360d5b9a4a52/actiontext/app/helpers/action_text/tag_helper.rb#L14). Passing a `class` argument overrides `trix-content`, removing the default styles.
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to rely on the API docs to convey this information instead. The user should already be checking the API docs to see if a :class option is supported, since there is no indication of it otherwise. Though we could make the information more easily accessible with a link:

diff --git a/guides/source/action_text_overview.md b/guides/source/action_text_overview.md
index 34d1deba6e..922161cc9d 100644
--- a/guides/source/action_text_overview.md
+++ b/guides/source/action_text_overview.md
@@ -85,7 +85,7 @@ end
 
 **Note:** you don't need to add a `content` field to your `messages` table.
 
-Then refer to this field in the form for the model:
+Then use [`rich_text_area`] to refer to this field in the form for the model:
 
 ```erb
 <%# app/views/messages/_form.html.erb %>
@@ -114,6 +114,8 @@ class MessagesController < ApplicationController
 end
 ```
 
+[`rich_text_area`]: https://api.rubyonrails.org/classes/ActionView/Helpers/FormHelper.html#method-i-rich_text_area
+
 ## Rendering Rich Text content
 
 Action Text will sanitize and render rich content on your behalf.

Copy link
Member

Choose a reason for hiding this comment

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

@stevepolitodesign I noticed you marked this conversation as resolved, but did not apply the change. Was that intentional? Do you have any concerns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathanhefner sorry about that, I'm still a PR n00b. I just made those changes.

Thank you for your patience on this!

Copy link
Member

Choose a reason for hiding this comment

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

@stevepolitodesign No worries! To clarify, though, I meant add the API docs link instead of the above paragraph. Generally, the guides provide a high-level overview, while the API docs delve into details such as default option values. Trying to include too many details in the guides would eventually make them unwieldy (both to read and maintain), and would duplicate a lot of the work of the API docs.

Also, when you're satisfied with all the changes, would you mind squashing the commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathanhefner thank you for the clarification. I went ahead and removed the paragraph per your suggestion. If this looks good I can squash these down.

@zzak
Copy link
Member

zzak commented May 28, 2021

@stevepolitodesign Still interested in wrapping this up? 🙇

Copy link
Contributor Author

@stevepolitodesign stevepolitodesign left a comment

Choose a reason for hiding this comment

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

@zzak I just added the suggestions made by @jonathanhefner. Let me know what else you need from me.

@@ -97,6 +97,8 @@ Then refer to this field in the form for the model:
<% end %>
```

**Note:** The `rich_text_area` form helper automatically adds the `trix-content` CSS class to the Trix editor element. `trix-content` is responsible for the editor's [default styling](https://github.com/rails/rails/blob/adc5eb66f861a98da32f7c682155360d5b9a4a52/actiontext/app/helpers/action_text/tag_helper.rb#L14). Passing a `class` argument overrides `trix-content`, removing the default styles.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathanhefner sorry about that, I'm still a PR n00b. I just made those changes.

Thank you for your patience on this!

@jonathanhefner jonathanhefner changed the title Add note regarding "trix-content" class. Add note regarding "trix-content" class. [ci-skip] May 29, 2021
@jonathanhefner
Copy link
Member

@stevepolitodesign It looks like an extra space got added after "Setting": https://github.com/rails/rails/pull/41889/checks?check_run_id=2701307222#step:6:10

@stevepolitodesign
Copy link
Contributor Author

@jonathanhefner I might have complicated things by merging the latest changes from rails:main into my PR. Let me know what I need to do to clean this up.

@zzak
Copy link
Member

zzak commented May 30, 2021

@stevepolitodesign Crap, yeah this is tough.

One idea is to just git reset --hard and then apply this diff.

@jonathanhefner
Copy link
Member

@stevepolitodesign I force-pushed to your branch (I hope you don't mind) to fix the rebase and squash the commits. Thank you for working on this!

@jonathanhefner jonathanhefner merged commit c831dea into rails:main May 30, 2021
@zzak
Copy link
Member

zzak commented May 30, 2021

Nice work @jonathanhefner! 👏

@stevepolitodesign
Copy link
Contributor Author

@jonathanhefner @zzak thank you both for helping me with this PR. I’m still pretty new to the open source community, and this was a very valuable experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copying ActionText associations between models creates an extra "trix-content" wrapper div
4 participants