#10910 closed task (blessed) (fixed)
Standardize comment form
Reported by: | Otto42 | Owned by: | beaulebens |
---|---|---|---|
Milestone: | 3.0 | Priority: | normal |
Severity: | normal | Version: | 2.9 |
Component: | Comments | Keywords: | has-patch |
Focuses: | Cc: |
Description
Every theme includes a substantially similar piece of code: the comments form. Largely, this form goes unchanged, except in extremely minor ways, from theme to theme. It's ripe for being pulled out of the theme and standardized in the core.
Points of note:
- The form contains all the same elements (name, email, url, comment text), which must have all the same names in order for the comment form to actually work.
- Virtually every theme uses styling to adjust the form, not changes to the HTML.
- Any plugin which enhances comments has to have code manually inserted into the theme, in most all cases. A more standardized form could have useful action hooks around the elements in the form, letting plugins work on the form in a simpler way (adding fields to sane locations, etc).
I suggest that this proceed in phases:
- Copy the comments form code from default and classic themes into the core, and replace it with a function call. comment_form(), for example.
- Add action hooks everywhere in it. Maybe some filters on the minor amounts of text in it, if it makes sense to do so. Add to these as needed, as the theme authors complain about not being able to easily adjust the HTML anymore. Should be a minority, almost all themes leave this verbatim from whatever theme they started out with.
- Encourage themes to adjust. Maybe start suggesting themes in the Expand section to use it, like threaded comments did.
Attachments (14)
Change History (62)
#2
follow-ups:
↓ 4
↓ 5
@
15 years ago
- Cc alexdunae added
I agree with exposing the comment form from the default theme as a function in core, just for simplicity's sake, but I strongly disagree with only letting theme developers customize the form via hooks.
In the past I've added multiple fields to comment forms (marketing opt-in, hometown, etc...), removed the URL field and reduced the comment textarea to input[type=text]. I also frequently re-write the HTML to put the labels above the fields. To accomplish all that, I'd have to 'complain' to get a lot of hooks added.
So while it would be nice to be able to pull up a standard form when needed, it needs to be tempered with flexibility for theme creators.
#4
in reply to:
↑ 2
@
15 years ago
Replying to alexdunae:
In the past I've added multiple fields to comment forms (marketing opt-in, hometown, etc...), removed the URL field, and reduced the comment textarea to input[type=text]. I also frequently re-write the HTML to put the labels above the fields. To accomplish all that, I'd have to 'complain' to get a lot of hooks added.
Nope. One filter hook can let you add labels, fields, whatever you like. Or remove existing ones, if done properly.
Consider every "field" to be in an array that looks like this:
array(
'name'=>'<label for="author">Name</label><input type="text"..blah blah..',
'email'=>'<label for="email">Email address</label><input type="text"..blah blah..',
.. blah blah...
)
Now, you run a filter on that array. Adding a new field is as simple as inserting a new element into the array. Delete an element from it and it disappears from the form. Want to redefine one? Just change the HTML in that one. Heck, you can even reorder the array if you like. One filter hook.
After the filter runs, a simple loop outputs the HTML code.
#5
in reply to:
↑ 2
@
15 years ago
Replying to alexdunae:
I also frequently re-write the HTML to put the labels above the fields.
You should do this with styling, not with changing the HTML. If your labels are in label tags and the inputs are, well, inputs, then some simple CSS will let you control their placement. No HTML editing required.
#6
@
15 years ago
- Milestone changed from Future Release to 3.0
- Type changed from enhancement to task (blessed)
@
15 years ago
First pass at adding a function for a standardized comment form. Totally open to actions/filters that should be in here
@
15 years ago
Implements the other attached patch to demonstrate some flexibility with the new comments form function on the default theme.
#10
@
15 years ago
Forgot to mention that the "test-on-default-theme.diff" patch also requires the patch on #11172 to provide an inline login form.
#13
follow-up:
↓ 14
@
15 years ago
I'm not really fond of having more globals. Can comment_form() call wp_get_current_commenter() and get_option('require_name_email') itself?
#14
in reply to:
↑ 13
@
15 years ago
Replying to ryan:
I'm not really fond of having more globals. Can comment_form() call wp_get_current_commenter() and get_option('require_name_email') itself?
Good call ryan. The updated patch (10910.002.diff) does the following:
- Uses wp_get_current_commenter() and get_option('require_name_email') instead of globals
- Moves to a much more "arg-centric" approach, rather than individual filters on everything
- Makes more of the form controllable via args (HTML ids, labels etc)
- Uses site_url() for the form action
- Adds a do_action('comment_form_top') right after the <form> tag
- Moves the comment_form_before/after actions to only happen when comments are open, and adds a comment_form_comments_closed action which is triggered if comments are closed
Also adding the test-on-default-theme.002.diff file to work with this version, and patch 004 over on #11172, still showing some simple use of the new hooks, including the action for when comments are closed (close comments on a post to try it out).
#16
@
15 years ago
two notes on the top of my head, on 10910.002.diff:
- could we add apply_filters(the_permalink, get_permalink()) where applicable?
- could we pass login/logout urls (complete with redirect parameters) to the login/logout captions? (Also, I think I only saw login captions; logout would be useful too.)
#17
@
15 years ago
Attaching a revised patch with the apply_filters call on the permalink.
If you want to adjust login/logout urls you can just filter the string must_log_in and logged_in_as and replace the whole thing pretty easily. I guess we could add another filter there, but seems excessive.
#20
follow-up:
↓ 21
@
15 years ago
Shouldn't this line:
<?php do_action( 'comments_form', $post_id ); ?>
Be:
<?php do_action( 'comment_form', $post_id ); ?>
#21
in reply to:
↑ 20
;
follow-up:
↓ 22
@
15 years ago
Replying to greenshady:
Shouldn't this line:
That was carried over from the code in the previous wp-content/themes/default/comments.php (and also in Sandbox) in an attempt to maintain compatibility with any themes or plugins already relying on that action.
#22
in reply to:
↑ 21
;
follow-up:
↓ 23
@
15 years ago
Replying to greenshady:
Shouldn't this line:
That was carried over from the code in the previous wp-content/themes/default/comments.php (and also in Sandbox) in an attempt to maintain compatibility with any themes or plugins already relying on that action.
That's my point exactly. From the current default, classic, and pretty much all other theme comments.php
files:
<?php do_action('comment_form', $post->ID); ?>
#23
in reply to:
↑ 22
@
15 years ago
Replying to greenshady:
That's my point exactly. From the current default, classic, and pretty much all other theme
comments.php
files:
Bah! Good point, my bad. I'll see if I can get that fixed and/or add a patch to here if required. Thanks for catching it.
#24
follow-up:
↓ 26
@
15 years ago
Can we get the default theme modified to use this, to show people how it should work?
#26
in reply to:
↑ 24
;
follow-up:
↓ 28
@
15 years ago
Replying to Otto42:
Can we get the default theme modified to use this, to show people how it should work?
Yeah, I like the sound of what's happening! ;) So are we still going to need to call comments_template('',true);
from single/index/page.php (whatever) or if we switch that for comment_form();
will it bring in the standard comment form that's been talked about, which I think might just be the case? - But just so I'm clear! :)
@
15 years ago
Changes all markup to match the new default theme (2010) and adds the aria-required attributes back in where relevant. Also introduces comment_notes_before and _after actions.
#28
in reply to:
↑ 26
@
15 years ago
Replying to markmcwilliams:
Yeah, I like the sound of what's happening! ;) So are we still going to need to call
comments_template('',true);
from single/index/page.php (whatever) or if we switch that forcomment_form();
will it bring in the standard comment form that's been talked about, which I think might just be the case? - But just so I'm clear! :)
Please do just ignore me, I got that 'ahhhh' moment, and while it wasn't quite what I expected in that respect, I do like having the standard area for people to write their comment and post it! :) I'm glad Otto created the ticket, so, thanks!
#29
@
15 years ago
Ugh! Why did it get changed to use all these non-semantic DIVs? The P's were correct markup, these DIVs are meaningless garbage. (The classes and arias are irrelevant and can go either way.)
If the 2010 theme actually looks like this, then the theme is fundamentally broken and needs to be fixed. We don't need to mess up the core code in this manner. Core should produce meaningful markup, not DIV-studded gibberish.
#30
@
15 years ago
As a theme author, the last thing I want to do is tell 1,000s of users, "Hey, look at this cool comment form function WP introduced. Yep, that's why your comments section is broken after upgrading."
If we're going to introduce markup, theme authors need to be able to override it. I'm specifically talking about the comment form title and the cancel comment reply link. Personally, I'd rather just have a callback function for creating my own comment form since I'm already filtering everything anyway.
And, what's with all the divs? Do we really need a <div>
wrapping every single element of the form? I also agree with Otto about using <p>
instead.
Patch coming that's a bit leaner. At the very least, take some ideas from it and make sure that we're not forcing theme authors into using specific markup.
#31
@
15 years ago
Also, all that "twentyten" stuff shouldn't be in there at all.
I think some of the basic ideas behind this have been lost. The goal here is to make a generic comment form that can be rather easily plugged into any and every theme.
-1 to the callback idea though. Callbacks just make things harder to standardize. Look at how callbacks have completely ruined wp_list_comments. If theme authors don't want to use the function, then they don't have to.
But, in order to make theme authors WANT to use the function, then it needs to fill a need and make life easier. This is overly complicated, IMO.
#32
follow-up:
↓ 33
@
15 years ago
Ugh, even worse, the classes and id's are all wrong. This is non-semantic in every possible way. "form-section"? "form-label"? Are you serious? Just awful. Worst markup ever.
Here's a patch that's a bit closer to how it should be done. Not suggesting this as final, just throwing it out there as an example.
#33
in reply to:
↑ 32
;
follow-up:
↓ 34
@
15 years ago
Replying to Otto42:
Ugh, even worse, the classes and id's are all wrong. This is non-semantic in every possible way. "form-section"? "form-label"? Are you serious? Just awful. Worst markup ever.
I didn't introduce those classes and IDs. I just used the ones already committed.
#34
in reply to:
↑ 33
@
15 years ago
Replying to greenshady:
I didn't introduce those classes and IDs. I just used the ones already committed.
I wasn't blaming anybody in particular. Just saying how wrong they are. :)
The ID's are wholly unnecessary. Classes are enough. You'd only need IDs if you're going to have more than one comment form on the page.
"form-section" and "form-label"? Come on. How generic can you get? The names of these things are supposed to be descriptive. If I wanted to address all the labels in forms using CSS, then there's a perfectly good way to do that: form label { whatever:xx; } Merely reproducing HTML in class-names is bloody stupid.
You really do need a wrapper around the fields. What if I want to hide the fields after an alternative login mechanism (Facebook, for example)? comment_form_before_fields and comment_form_after_fields are find for this, all you need is a div wrapper.
The 2010 theme as a whole has these same basic problems in several places, but those are better expressed in other tickets.
@
15 years ago
The attached patch takes a swing at fixing the semanticness of the comment_form output, based largely on Otto42's patch. Also includes all the CSS modifications required for 2010 to look the same as it does currently with the new defaults form output.
@
15 years ago
This patch modifies the previous default/Kubrick to use the new comment_form() function, set up to output the same HTML as it did previously, with CSS to make it look the same where necessary. It's not *quite* perfect, but it's very, very close to the same HTML.
#38
@
14 years ago
I attach a patch that fixes two potentially confusing typos in the phpDoc block, and also suggests renaming two action hooks:
comment_form_after_fields comment_form_before_fields
to:
comment_form_fields_after comment_form_fields_before
I think the suggested order is more logical.
Another thing I don’t like in the comment form is the markup for the allowed HTML elements: I think the Definition List is not the best choice here semantically, and it will also force people to use extra CSS to make that block look like one single paragraph, as it is most common in current usage. (Twenty Ten does exactly this.)
Why not go with the simplest option for the default? (Which people can then change easily if they so wish.) Something like:
'<p class="form-allowed-tags"> . __( 'You may use... blah blah...:' ) . '<code>' . allowed_tags() . '</code>' . </p>
Other than these minor niggles, I really enjoyed playing with comment_form
. Thanks and congratulations to all who contributed!
@
14 years ago
In HTML5, small element represent different meaning from HTML4. Besides, small element probably the most hated element in the history of kubrick theme. Switching to span. Adds new parameter $before, $after to cancel_comment_reply_link(). Also checks when needed.
#39
follow-up:
↓ 41
@
14 years ago
demetris, I would upload a diff of the allowed tags issue if you feel strongly about it.
#41
in reply to:
↑ 39
@
14 years ago
Replying to nacin:
demetris, I would upload a diff of the allowed tags issue if you feel strongly about it.
Done! :-)
#42
@
14 years ago
Ok. It looks like beaulebens added the definition list markup when going over Otto42's suggestion of more semantic markup. I'd rather hear his reasoning before jumping to change it.
#43
@
14 years ago
I switched to that just because it seemed to make sense semantically as far as this being a matching label/definition pair. I don't mind either way, so if people are bothered by it, then we can change it just as easily.
#44
@
14 years ago
Not sure I see the point of using a definition list here, as it's not really a list nor providing definitions. So I don't see the semantic sense in it.
+1 for switching to a simpler paragraph method.
Additional: Could we wrap the top fields in a fieldset? For separation purposes, it'd be easier to style a fieldset than trying to use those actions to add one, especially for multiple plugins to access it.
#45
@
14 years ago
Any dev feedback for http://core.trac.wordpress.org/attachment/ticket/10910/10910-cancel_comment_reply_link.patch?
Just a thought, wouldn't it be nice to move the cancel reply out of the heading. Probably placing it after the comment form heading (before comment-notes) or next to Submit Comment.
+1
I'd love to be able to easily insert a login form or login prompt in the comment form area on wordpress.com. If we changed all themes to use a well-hooked comment_form(), this would be simple.