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

Update rendering.md #1557

Closed
wants to merge 3 commits into from
Closed

Update rendering.md #1557

wants to merge 3 commits into from

Conversation

Jwan622
Copy link
Contributor

@Jwan622 Jwan622 commented Mar 5, 2016

Purpose

Changes

Caveats

Related GitHub issues

Additional helpful information

-adding some documentation regarding overriding the root node.

-adding some documentation regarding overriding the root node.
@@ -155,7 +155,7 @@ PR please :)
The resource root is derived from the class name of the resource being serialized.
e.g. `UserPostSerializer.new(UserPost.new)` will be serialized with the root `user_post` or `user_posts` according the adapter collection pluralization rules.

Specify the root by passing it as an argument to `render`. For example:
When using the JSON adapter in your initializer (ActiveModelSerializers.config.adapter = :json), you can specify the root by passing it as an argument to `render`. For example:
Copy link
Member

Choose a reason for hiding this comment

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

When using the JSON adapter in your initializer (ActiveModelSerializers.config.adapter = :json)

You can also use render adapter: :json to specify the adapter, so I think in your initializer (ActiveModelSerializers.config.adapter = :json) might be misleading here.

Maybe it would be more clear to start the section by stating that root only applies to JSON adapter?

@remear
Copy link
Member

remear commented Mar 8, 2016

@Jwan622 Are you interested in making these suggested changes?

@Jwan622
Copy link
Contributor Author

Jwan622 commented Mar 8, 2016

Yes!!

Sent from my iPhone

On Mar 8, 2016, at 3:10 PM, Ben Mills notifications@github.com wrote:

@Jwan622 Are you interested in making these suggested changes?


Reply to this email directly or view it on GitHub.

@bf4
Copy link
Member

bf4 commented Mar 14, 2016

@Jwan622 wanna bring this one home?

@remear
Copy link
Member

remear commented Mar 15, 2016

@Jwan622 👍 Can you squash these commits. I'll merge this once that's done.

@Jwan622
Copy link
Contributor Author

Jwan622 commented Mar 15, 2016

How... Do I do this? By squash you mean just turn it into one commit? I used the GUI...

Sent from my iPhone

On Mar 15, 2016, at 12:43 PM, Ben Mills notifications@github.com wrote:

@Jwan622 Can you squash these commits. I'll merge this once that's done.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1557 (comment)

@remear
Copy link
Member

remear commented Mar 15, 2016

remear added a commit to remear/active_model_serializers that referenced this pull request Mar 17, 2016
remear added a commit that referenced this pull request Mar 17, 2016
@remear
Copy link
Member

remear commented Mar 17, 2016

@Jwan622 I rebased it for you, added this to the CHANGELOG and merged it into master. Thanks for doing this!

@remear remear closed this Mar 17, 2016
@Jwan622
Copy link
Contributor Author

Jwan622 commented Mar 29, 2016

Thanks! Can you rebase squash using the github gui? Or does it have to be done command line?

@NullVoxPopuli
Copy link
Contributor

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.

5 participants