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

WIP: Move templates out of options #194

Merged
merged 2 commits into from May 18, 2016
Merged

WIP: Move templates out of options #194

merged 2 commits into from May 18, 2016

Conversation

scossar
Copy link
Contributor

@scossar scossar commented May 15, 2016

Based on this commit to the 10up fork of wp-discourse: https://github.com/10up/wp-discourse/commit/5c9d43c4333e136204d5a3b07192f4b368c3f518

This PR moves the html templates out of the plugin's options. If this is implemented, html templates will no longer be customizable through the options page. They can be customized through the theme using the filters that are applied to each function. A link to the documentation of how to do that is added to the settings page. Here is a tentative version: https://github.com/scossar/wp-discourse/wiki/Template-Customization

This PR also changes the plugin's text-domain from discourse to wp-discourse.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @retlehs, @iamntz and @santouras to be potential reviewers

</div><!-- #respond -->
</div>
<?php
return apply_filters( 'discourse_replies_html', ob_get_clean() );
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it's better to have ob_get_clean called outside the apply_filters (here and everywhere else).

My reasoning is this: if you want only to append some extra markup, you'll have to remember to clean the buffer on return. I think it's an unnecessary extra step...

@retlehs
Copy link
Contributor

retlehs commented May 17, 2016

👍 instead of linking to the wiki on your fork, can you please go ahead and update the links to be based off this repo?

and since we require php 5.3, can you add a namespace to the top of that HTMLTemplates class?

@scossar
Copy link
Contributor Author

scossar commented May 17, 2016

Sure. Can I create the wiki on here?

@scossar
Copy link
Contributor Author

scossar commented May 17, 2016

I've added a namespace and rebased this with the current master branch. I still need to update the links. I can change the namespace if there's something you would rather use than WPDiscourse.

@retlehs
Copy link
Contributor

retlehs commented May 17, 2016

namespace looks good, and yeah you can go ahead and add to the wiki on here

@retlehs
Copy link
Contributor

retlehs commented May 18, 2016

tested this out and it looks good! thanks for doing this.

will merge once you've rebased and tag a new version soon

@retlehs
Copy link
Contributor

retlehs commented May 18, 2016

still need the wiki URLs updated to this repo's 👀

mention me/reply once that's done and i'll merge

@erlend-sh
Copy link
Contributor

I had a brief discussion about this with @scossar in a separate channel, so I'll repost here:

If it makes the plugin less error prone that'd be a big win, but at the same time losing the ability to easily style a theme using the HTML Template boxes would be a pretty great loss imo. In my side-project one of the volunteer devs used these templates to great effect. If he had to mess around with a functions.php file, I'm not so sure he would have bothered taking the time aside to style our comments.

What's your take on this @retlehs ? This does make theme tweaking more of a developer-y action.

@retlehs
Copy link
Contributor

retlehs commented May 18, 2016

customizing HTML templates is developer-y to begin with, and having the markup editable in the admin is awkward. it also doesn't make sense for that HTML to be saved to the options table in the database.

@scossar
Copy link
Contributor Author

scossar commented May 18, 2016

@retlehs I've updated the URLs

@retlehs retlehs merged commit 7e73346 into discourse:master May 18, 2016
@scossar scossar deleted the move-templates-out-of-options branch July 22, 2016 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants