-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Improve Display #377
Improve Display #377
Conversation
Shouldn't we work on templating instead of adding so much features/settings for the "old world"? I think it would make all the changes in this PR obsolet. /cc @chrisbergr |
Why I am not a big fan of overwriting the In the current PR we only replace the template for Webmentions (which seems not 100% compatible to that in core), which means Webmentions and Comments look slightly different: If WordPress changes the default behaviour and/or the CSS changes, it might break Webmention representation. Most of the themes do not rely on the comment walker template at all and implement their own representation using the comment callback. See the autonomie theme for example: |
It was designed to accommodate themes that don't use the comment callback. If you use it, that would take precedence. The same issue would be the case with @chrisbergr 's pr. Which can be implemented by moving this code to template files. |
The goal is to create a good default presentation things could override. |
But then we should at least stick with the default, so that there are no CSS differences (see my screenshot). |
But the html5_comment version I included has no differences on the CSS by design. It adds microformats classes and the citation line that we had in Semantic Linkbacks. |
But why the differences then? I will try to debug the code... maybe the theme add some CSS using microformats classes :( |
Everything in this implementation, as well as the new comment implementation for 5.0 was designed to be overridden the way it always was. The new way of showing the reactions loads them, then loads the theme's comment display afterward. The comment walker overrides start_el in order to allow loading additional callbacks and additional options to wp_list_comments so it is backward compatible. It allows the theme to still use its own callback... |
The part I don't understand is that #364 overrides html5_comment as well, but you approved that, and I want to move in that direction with a second PR. |
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
@pfefferle @chrisbergr Added a FAQ to explain how the existing system works. While the html5_comment addition happens in this plugin, everything else I noted was already in version 5. I think it needs it. If we expand to add a locate_template option, it would simply be another alternative. |
The default template I provided uses the original code from WordPress. That's why CSS changes should not be a problem. If they change something on their html structure, it is the same problem as with any other thing that works with templates. It is not a problem that we then offer an update of the standard template. If you have ever worked with WooCommerce and have overwritten there email templates: They solve this quite nicely by noticing you that there have been changes and your template is no longer up to date.
This is not an issue for this topic, it is more general. If the theme uses a custom walker, the walker provided by this plugin won't be used at all. |
@pfefferle Wanted to see about remerging this. |
The problem I still have with this PR is, that it will always overwrite the comment representation of the theme, and I have no chance to prevent that. I would love to at least have a theme_support check or something similar. |
No, it's the opposite, the theme will always overwrite it. |
But in what case will the comment-walker version be used then? |
When the theme doesn't set a custom rendering, or when the theme opts to use the built in. |
It is not directly because of this PR, but I see an error (on block themes) in the comments section:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This addresses a variety of display issues, specifically.
This sets up us moving into adding in #364.