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

Remove HTMLTimeElement: dateTime syntax table, move it to <time> #35453

Merged
merged 4 commits into from
Aug 15, 2024

Conversation

Josh-Cena
Copy link
Member

This table is poorly marked up and very inconsistent. It also shouldn't belong here but should belong in the <time> reference which is the canonical place for HTML attribute values.

@Josh-Cena Josh-Cena requested review from a team as code owners August 13, 2024 19:52
@Josh-Cena Josh-Cena requested review from Elchi3 and estelle and removed request for a team August 13, 2024 19:52
@github-actions github-actions bot added Content:HTML Hypertext Markup Language docs Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed labels Aug 13, 2024
Copy link
Contributor

github-actions bot commented Aug 13, 2024

Preview URLs

(comment last updated: 2024-08-15 20:19:59)

Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the table! Agree with moving it to the canonical element page.

I spotted a few nits you might want to take a quick look at.

files/en-us/web/api/htmltimeelement/datetime/index.md Outdated Show resolved Hide resolved
@@ -34,46 +34,131 @@ The _datetime value_ (the machine-readable value of the datetime) is the value o

### Valid datetime values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Valid datetime values
### Valid `datetime` values

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @dipikabh, when we loosened the heading typesetting guidelines, I made sure it says "you may use code formatting, but you are not required to". My agreement to the initial proposal was based on the intention of disambiguation, and when there's no ambiguity, I would prefer not to use code in headings.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the contrary, I somehow remember the consensus leaning more towards adding the code formatting backticks in headings, and I believe I've also been suggesting those in my reviews.

I went looking — from Heading levels formatting guideline in our style guide and #22472 to https://github.com/orgs/mdn/discussions/271. I see the guideline is loose when it says "you can use backticks for code terms".

I agree, with datetime, there's no ambiguity, but it's a term we always format with backticks in prose, so it would make sense to extend the same treatment to it in the heading as well.

To me, it's just simpler and easier to consistently use backticks around names of properties, values, interfaces in headings (the guideline is not against it) without needing any other criterion. I'm not sure if everyone is using the same approach, that is ambiguous vs unambiguous, when deciding whether or not to use backticks. All the same, I won't insist on the change here but might be worth considering.

Copy link
Member Author

Choose a reason for hiding this comment

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

My decision on this is because H1 headings can't have code formatting anyway, so we already have code-stripping happening for top-level headings.

Copy link
Contributor

Choose a reason for hiding this comment

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

H1s never figured in my mind as part of this discussion! :) It's only been about the section headings.

files/en-us/web/html/element/time/index.md Outdated Show resolved Hide resolved
files/en-us/web/html/element/time/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Josh-Cena
Copy link
Member Author

Looks like we converged on something we can agree on, so I'm going to self-merge it. Thanks for the reviews

@Josh-Cena Josh-Cena merged commit 835c199 into mdn:main Aug 15, 2024
8 checks passed
@Josh-Cena Josh-Cena deleted the fix-table branch August 15, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTML Hypertext Markup Language docs Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants