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

Make comments render methods hookable #1958

Open
gebeer opened this issue Aug 3, 2016 · 4 comments
Open

Make comments render methods hookable #1958

gebeer opened this issue Aug 3, 2016 · 4 comments

Comments

@gebeer
Copy link

gebeer commented Aug 3, 2016

Please make the render functions for the comments list and item in CommentList.php hookable.
I need microdata in the markup for the comment lists ratings. I know I could loop through the comments and output my own markup. But it would be much better if I could place this in a module and hook from there as I (and maybe others) sure could use this on other projects, too.
See also forum tread
Thank you.
I added a pull request for master

@LostKobrakai
Copy link

What about not replacing the method, but adding a custom one?

$wire->addHook('CommentList::renderWithMicrodata', function($e){ … });

Most of the class's methods are public anyways.

@gebeer
Copy link
Author

gebeer commented Aug 3, 2016

haven't thought about that. But now you point me to it, it seems like a great idea ;)

Are those methods not hookable for a certain reason?

@ryancramerdesign
Copy link
Owner

Are those methods not hookable for a certain reason?

We don't usually make methods hookable unless there's a definite need for it. That's because there's more overhead in calling a hookable method vs. a non-hookable method. However, after looking at LostKobrakai's suggestion, if you find that you still need these methods to be hookable, just let me know.

@gebeer
Copy link
Author

gebeer commented Aug 6, 2016

Thank you, Ryan, for clarifying. I think I can do with LostKobraKai's suggestion. So there isn't really a need to make those methods hookable and you can close this issue.

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

No branches or pull requests

3 participants