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.

Sign up for GitHub

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

Jump to bottom

fix: remove ending periods from non-sentence list items #23845

Merged
merged 5 commits into from
Mar 27, 2023
Merged

fix: remove ending periods from non-sentence list items #23845

merged 5 commits into from
Mar 27, 2023

Conversation

de-oz
Copy link
Contributor

@de-oz de-oz commented Jan 23, 2023

Description

Removed periods from the list items which do not represent complete sentences under the See also heading.

@de-oz de-oz requested a review from a team as a code owner January 23, 2023 20:15
@de-oz de-oz requested review from jpmedley and removed request for a team January 23, 2023 20:15
@github-actions github-actions bot added the Content:WebAPI Web API docs label Jan 23, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jan 23, 2023 •
edited
Loading

Preview URLs (33 pages)
Flaws (14)

Note! 32 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/HTMLMediaElement
Title: HTMLMediaElement
Flaw count: 14

  • macros:
    • /en-US/docs/Web/API/HTMLMediaElement/played does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/preload does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/seeking does not exist
    • /en-US/docs/Web/API/HTMLMediaElement/onencrypted redirects to /en-US/docs/Web/API/HTMLMediaElement
    • /en-US/docs/Web/API/HTMLMediaElement/onwaitingforkey redirects to /en-US/docs/Web/API/HTMLMediaElement
    • and 9 more flaws omitted
External URLs (2)

URL: /en-US/docs/Web/API/HTMLCanvasElement/getContext
Title: HTMLCanvasElement.getContext()

(comment last updated: 2023-03-26 17:38:44)

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jan 23, 2023 •
edited
Loading

I don't have a problem with this, but where is the style guide it is meeting?

I.e. in https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide I see mostly instruction to use punctuation. For me consistency is more important than punctuation, but following this guide is most important of all.

@de-oz
Copy link
Contributor Author

de-oz commented Jan 24, 2023

I don't have a problem with this, but where is the style guide it is meeting?

I.e. in https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Writing_style_guide I see mostly instruction to use punctuation. For me consistency is more important than punctuation, but following this guide is most important of all.

This PR was based on consistency rather than the style guide, as usually on MDN list items under the See also section do not include periods if they aren't complete sentences and only contain links. If both including a period and omitting it in such cases are acceptable, then I guess this PR can be closed, but if not, please let me know how this inconsistency should be handled.

@hamishwillee
Copy link
Collaborator

please let me know how this inconsistency should be handled.

Of course. @Rumyra Can we have direction on whether see also links bullet lists should have punctuation or not. Then we can update the style guide (or at least know the rule). FWIW I would prefer no punctuation, because often these will be single words or heading names that are not naturally complete sentences.

@dipikabh
Copy link
Contributor

Hi @hamishwillee and @de-oz, hopefully the guidelines for 'See also' and the punctuation to be used with the links will soon be out. Stay tuned.

@jpmedley jpmedley removed their request for review February 23, 2023 19:42
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Mar 12, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@hamishwillee
Copy link
Collaborator

Hi @dipikabh, Did final rules get agreed here?

@teoli2003
Copy link
Contributor

teoli2003 commented Mar 13, 2023 •
edited
Loading

The short answer is yes. The final PR to get them in the meta docs is waiting for a last round of approval to mark consensus (your approval to show you adhere to the soon-confirmed consensus is welcome): #25191

I'm really excited by the work of @dipikabh to unify this section!

@hamishwillee
Copy link
Collaborator

Yes, I had a couple of reservations (mostly matters of "taste"), but it is a significant improvement.

Looks like see also are mostly bulleted lists, where items are phrases with punctuation. So this PR would not be accepted. There is some inconsistency between the instructions and representative examples so I'll wait until it merges before proceeding with this.

Thanks!

dipikabh
dipikabh requested changes Mar 23, 2023
View reviewed changes
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.

Edited two files to make the list items follow the new "See also" section guidelines.

In general, the following structure is being followed:
<link text>: Description phrase(no punctuation)
The description phrase is optional.

So yes, removing periods here is correct (because they're not complete sentences) but also rephrase the list item so that the link text is at the beginning.

files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/height/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/height/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/height/index.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Mar 24, 2023
@de-oz de-oz requested a review from dipikabh March 24, 2023 13:00
dipikabh
dipikabh requested changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@dipikabh dipikabh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fixes! Suggesting a few more edits, including some untouched list items

files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/canplaytype/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlcanvaselement/getcontext/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/seekable/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/texttracks/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/htmlmediaelement/texttracks/index.md Outdated Show resolved Hide resolved
files/en-us/web/api/mediaerror/message/index.md Outdated Show resolved Hide resolved
@de-oz de-oz requested a review from dipikabh March 26, 2023 08:14
@de-oz de-oz mentioned this pull request Mar 26, 2023
Merged
@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Mar 26, 2023
@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Mar 26, 2023
dipikabh
dipikabh approved these changes Mar 27, 2023
View reviewed changes
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 👍

@dipikabh dipikabh merged commit 45e4833 into mdn:main Mar 27, 2023
@hamishwillee
Copy link
Collaborator

Thanks. Great to get this in. Now I just have to make my review and writing habits match our agreed bullet list format :-)

@de-oz de-oz deleted the de-oz-patch branch March 28, 2023 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dipikabh dipikabh dipikabh approved these changes

Assignees
No one assigned
Labels
Content:WebAPI Web API docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@de-oz @hamishwillee @dipikabh @teoli2003