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

Jump to bottom

Add hook to modify rendered content without layout #4752

Closed
wants to merge 6 commits into from
Closed

Add hook to modify rendered content without layout #4752

wants to merge 6 commits into from

Conversation

johnkhughes
Copy link
Contributor

Lets you modify the content as HTML without affecting the HTML of the rest of the page. Addresses issue #4714.

@envygeeks
Copy link
Contributor

Renderer is handled by more than "posts", I like the premise but I think the name should be more generic, especially since it will be tapped by more than one type of document at the end of the day.

@johnkhughes
Copy link
Contributor Author

The "post" in :post_content_render means "after" rather than "article", if that's what you mean. I was trying to fit in with the names of the other hooks. But please feel free to suggest another name for it!

@envygeeks
Copy link
Contributor

:post_content (with content) changes its context. I think :post_render_content and :pre_render_content would be less confusing names. /cc @jekyll/core @jekyll/stability

@johnkhughes
Copy link
Contributor Author

Good point. I like :post_render_content. I'm not sure there is a need for :pre_render_content though; this is already covered by :pre_render a few lines earlier.

@envygeeks
Copy link
Contributor

LGTM. :shipit: /cc @jekyll/core @jekyll/stability @jekyll/build

@parkr
Copy link
Member

parkr commented Apr 14, 2016

What do you think about :post_convert?

@envygeeks
Copy link
Contributor

@parkr I like that too, even if it deviates from what we already have (but I think it fits.) Considering we could have potentially dozens of hooks that aren't exactly pre/post and just a single action it makes a lot of sense.

@johnkhughes
Copy link
Contributor Author

I don't mind what it's called. Is everyone happy with :post_convert? Seems pretty clear to me.

@envygeeks
Copy link
Contributor

I'm 👍 on it.

@parkr
Copy link
Member

parkr commented Apr 20, 2016

@johnkhughes post_convert sounds good to me. You'll also have to rename it in lib/jekyll/hooks.rb.

@johnkhughes
Copy link
Contributor Author

Slaps forehead

parkr
parkr reviewed Apr 22, 2016
View reviewed changes
lib/jekyll/renderer.rb
@@ -67,6 +67,10 @@ def run
output = convert(output)
document.content = output

# Hook to modify converted content before layout is applied
document.trigger_hooks(:post_convert, output)
output = document.content
Copy link
Member

@parkr parkr Apr 22, 2016 •
edited

Choose a reason for hiding this comment

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

I don't think this will do what you think it will do.

Can you write a test for this? In features/hooks.feature is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur, we shouldn't assume users are destructive and should instead encourage them to return their modified content, especially since we've an open ticket to start freezing all our strings, this will certainly impede that and delay it much further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parkr Possibly a stupid question, but how do I write a test?

@parkr parkr added this to the flexible milestone Apr 22, 2016
@parkr parkr added pending-feedback We are waiting for more info. feature labels Apr 22, 2016
@johnkhughes
Copy link
Contributor Author

I'm still interested in sorting this out if anyone can help get me started with writing a test for it.

@jekyllbot jekyllbot removed the pending-feedback We are waiting for more info. label May 18, 2016
@DirtyF
Copy link
Member

DirtyF commented May 18, 2016

@johnkhughes Did you take a look at https://github.com/jekyll/jekyll/blob/master/features/hooks.feature ?

ghost
ghost approved these changes Nov 22, 2017
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM!

@ghost ghost modified the milestones: flexible, v3.7.0 Nov 22, 2017
@ghost

This comment has been minimized.

Sign in to view
@DirtyF DirtyF removed this from the v3.7.0 milestone Dec 15, 2017
@DirtyF DirtyF added this to the flexible milestone Dec 15, 2017
@pyrmont
Copy link

pyrmont commented Mar 3, 2018

I'm interested in this feature. Is it waiting for anything before being merged in?

@pathawks pathawks modified the milestones: flexible, 4.0 Apr 30, 2018
@pathawks
Copy link
Member

Is it waiting for anything before being merged in?

It has an approved review, but does not have tests, so I am not sure what the status is.

@DirtyF
Copy link
Member

DirtyF commented Sep 24, 2018

@johnkhughes Are you still willing to ship that feature and add the missing tests?

@parkr
Copy link
Member

parkr commented Sep 24, 2018

I think it would be helpful to have this!

@jekyllbot jekyllbot mentioned this pull request Sep 24, 2018
Closed
@DirtyF
Copy link
Member

DirtyF commented Sep 24, 2018

@parkr As I couldn't apply the patch, I reported the changes manually in a new PR #7271

@DirtyF DirtyF closed this Sep 24, 2018
@johnkhughes johnkhughes mentioned this pull request Mar 29, 2019
Closed
@jekyll jekyll locked and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
Assignees
No one assigned
Labels
feature frozen-due-to-age needs-tests pending-resolution
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@johnkhughes @envygeeks @parkr @DirtyF @pyrmont @pathawks @jekyllbot @ashmaroli