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

Improve Web/API/Document/images #33659

Merged
merged 2 commits into from
May 19, 2024

Conversation

leon-win
Copy link
Member

Description

This PR updates Web/API/Document/images. List of changes:

  • replace link to HTMLImageElement instead of constructor,
  • improve example description,
  • remove unnecesary line breaks.

…ple description, remove unnecesary line breaks
@leon-win leon-win requested a review from a team as a code owner May 17, 2024 12:15
@leon-win leon-win requested review from wbamberg and removed request for a team May 17, 2024 12:15
@github-actions github-actions bot added Content:WebAPI Web API docs size/s 6-50 LoC changed labels May 17, 2024
Copy link
Contributor

github-actions bot commented May 17, 2024

Preview URLs

(comment last updated: 2024-05-19 23:58:06)

Copy link
Collaborator

@hamishwillee hamishwillee left a comment

Choose a reason for hiding this comment

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

Thanks @leon-win

I'll merge this one, but as a general rule MDN does not accept layout-only changes or "to my taste" changes on their own. The reason is that the changes do not affect the rendered content, but they do take time to review, and the time the reviewer team have is limited.

We do however accept technical fixes, and grammatical fixes etc that remove ambiguity, and so on. So you are more than welcome to add a technical fix, and as part of that also tidy layout.

@hamishwillee hamishwillee merged commit 0af9a58 into mdn:main May 19, 2024
8 checks passed
@hamishwillee
Copy link
Collaborator

Ah, @leon-win Reading this more carefully, your change was technical - removing the constructor link. Apologies.

W.r.t. line breaks there are no firm rule on how these should be done in MDN. Most of the reviewer team strip out line breaks within a sentence. I tend to split on sentences.

@leon-win leon-win deleted the improve/web-api-document-images branch May 20, 2024 09:59
@leon-win
Copy link
Member Author

Ah, @leon-win Reading this more carefully, your change was technical - removing the constructor link. Apologies.

W.r.t. line breaks there are no firm rule on how these should be done in MDN. Most of the reviewer team strip out line breaks within a sentence. I tend to split on sentences.

Hello, @hamishwillee! Thank you for review and comments!

Yes, I made two changes (about the constructor and about the example) and at the same time decided to remove line breaks))

I agree that this is a matter of taste.
In my opinion, using an autoformatter (if possible in this case) would eliminate ambiguity and reduce the burden on reviewers.

@hamishwillee
Copy link
Collaborator

In my opinion, using an autoformatter (if possible in this case) would eliminate ambiguity and reduce the burden on reviewers.

@leon-win IMO the ambiguity only matters if it affects the rendered output, which it does not. IMO not worth the churn to do automatically. This is one of those things where people have strong opinions: I'd be pretty happy if we all agreed to break on sentences, but if the agreement ended up being "break on 80chars" I would cease working on MDN :-)

@leon-win
Copy link
Member Author

@leon-win IMO the ambiguity only matters if it affects the rendered output, which it does not. IMO not worth the churn to do automatically. This is one of those things where people have strong opinions: I'd be pretty happy if we all agreed to break on sentences, but if the agreement ended up being "break on 80chars" I would cease working on MDN :-)

The 80 character limit is more suitable for the Python language)) Therefore, stay in MDN 🤗

And about moving sentences to a new line. To be honest, I don’t really like this, precisely because these line breaks are not in the resulting display. And if we want to make a semantic emphasis, to separate parts of the text, then we need to make a break in the form of a separate paragraph, it seems to me so.

But now, seeing such line breaks, I will remember our conversation and try not to delete them =)

@hamishwillee
Copy link
Collaborator

Appreciate you not deleting them. I have several reasons for this approach.

  1. Some years ago I was working with translators and they preferred sentence to paragraph breaks. That is probably tool-specific.
  2. When reviewing in Github it is more likely to be able to have fixes constrained to a sentence than a paragraph - that just makes seeing the changes easier. Smaller lines obviously make this even easier, but I don't like those.
  3. When I review, I read line by line, and it makes it easier for me to parse carefully. Again, this is a "me" thing.

I can live with paragraph breaks. There are many who are not convinced my approach is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/s 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants