Posted by andypost on May 24, 2010 at 1:44pm
8 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | markup |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs design review |
Issue Summary
Changing SPAN to VAR for time difference seems more semantically correct
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| comment-block-var-d7.patch | 1.46 KB | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-block-var-d7.patch. | View details |
Comments
#1
Tagging, reviewing
#2
The last submitted patch, comment-block-var-d7.patch, failed testing.
#3
Re-roll
#4
Yes, technically
<var>is a better choice. It's definitely one of those rarely used tags... I submitted the patch that got rid of the old<br/>tag and added the<span>, and at the time<var>didn't even come to mind.It definitely needs a space before the tag though, whether it is changed or not, so it's not totally squished against the comment title (my fault, sorry), so I'm changing this to a bug report.
#5
Sure, I forget that span produces a space before it.
So attached 2 patches. First with NBSP and second with SPACE
I think nbsp-patch is more reasonable because time should be linked to comment subject
#6
Sounds good to me. Thanks for the re-roll.
RTBC'ing 807744-comment-block-var-nbsp-d7.patch in #5.
#7
My understanding of the var-tag is that it is used to indicate a variable or a parameter for an application. It does not sound appropriate to use in this context. At a minimum it requires some more research and discussion.
#8
I had the same reaction initially. It's definitely not a popular tag, so I tried to find some actual implementations. Even though I didn't find any, I found a better definition and examples:
Programming context example:
At the bottom of the search results screen, we have added "Next <var>n</var>" functionality to allow the user to view more than one screen of search results.Mathematical and placeholder in prose examples:
<figure><math>
<mi>a</mi>
<mo>=</mo>
<msqrt>
<msup><mi>b</mi><mn>2</mn></msup>
<mi>+</mi>
<msup><mi>c</mi><mn>2</mn></msup>
</msqrt>
</math>
<figcaption>
Using Pythagoras' theorem to solve for the hypotenuse <var>a</var> of a triangle with sides <var>b</var> and <var>c</var>
</figcaption>
</figure>
<p>If there are <var>n</var> pipes leading to the ice cream factory then I expect at <em>least</em> <var>n</var> flavors of ice cream to be available for purchase!</p>The definitions are still pretty vague, but it's not just for application variables and in this case, it seemed correct that the date is a variable because of the way it's formatted, but maybe I'm wrong? Maybe just because the date is variable doesn't mean it is a variable? I'm confused now.
Anyway, I'm sorry I jumped the gun here. I should have solicited more reviews before marking this RTBC. I am tagging "needs design review" in case anyone can provide some input.
#9
The way I understand this,
<var>is for symbols (either mathematical or literal). It doesn't seem like the proper choice here.However, if we wrap this in a
<abbr>and add the full ISO8601 timestamp, we could have a nice microformat here.#10
Good idea Damien :)
Hopefully I did this correctly.
#11
Jacine, I think we need nbsp before ABBR do not allow breaking this place so time would always attached to comment title.
Except this RTBC
#12
Forgot about that. :)
#13
Looks very good to me.
#14
+1, RTBC
#15
But why? I'm happy to add a microformat, but it feels like we're adding it just because we can. We can thinker more about this in D8?
#16
@Dries There is no other reason, so waiting to do this in D8 sounds fine. However, we do have a bug here that needs to be fixed for D7. A space is needed before the opening span, otherwise the text is squished together. Here's a quick screenshot of the problem and patch that just adds the needed space.
Then we can switch this back to D8 with a more appropriate title.
#17
Let's commit this space and leave microformat for D8
#18
Committed to CVS HEAD. Thanks.
#19
Patch #16 accidentily lost D7 concat coding style...?
#20
Fix for code-style
#21
Thanks @andypost!
Ugh, I feel so bad. Sorry Dries! :(
#22
Committed to HEAD.
#23
Re-title for D8
#24
I think was already committed, not sure why its in needs review state.
#25
Automatically closed -- issue fixed for 2 weeks with no activity.