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

Convert Web/JavaScript to markdown #6936

Closed
wants to merge 2 commits into from

Conversation

fiji-flo
Copy link
Contributor

yarn md h2m web/javascript --locale en-US --mode replace

These are two commits and we must not squash merge. git is smart an decides it's cheaper to consider this a delete + add operation instead of rename + modify

@fiji-flo fiji-flo requested a review from wbamberg July 15, 2021 18:04
@fiji-flo fiji-flo requested a review from a team as a code owner July 15, 2021 18:04
@sideshowbarker
Copy link
Collaborator

sideshowbarker commented Jul 15, 2021

I’d like to express a strong preference here to not hard-wrap the Markdown output to any particular character width — that is, I’d strongly prefer we preserve the existing line lengths unwrapped, as they are in the current HTML sources.

But I see that this patch as currently written causes all the Markdown output to be hard-wrapped to 80 characters.

I don’t know if that completely intentional — and is one of the explicit design decisions already documented somewhere — or is instead just a default setting in the tooling that we’re not overriding; but regardless, I would like as for consideration that we don’t do that re-wrapping in Markdown output but instead preserve the original line lengths un-wrapped.

@wbamberg
Copy link
Collaborator

I’d like to express a strong preference here to not hard-wrap the Markdown output to any particular character width — that is, I’d strongly prefer we preserve the existing line lengths unwrapped, as they are in the current HTML sources.

But I see that this patch as currently written causes all the Markdown output to be hard-wrapped to 80 characters.

I don’t know if that completely intentional — and is one of the explicit design decisions already documented somewhere — or is instead just a default setting in the tooling that we’re not overriding; but regardless, I would like as for consideration that we don’t do that re-wrapping in Markdown output but instead preserve the original line lengths un-wrapped.

I believe this is being done by Prettier, and that it's possible to configure it. We haven't explicitly discussed line length but I'm happy to not wrap.

@hamishwillee
Copy link
Collaborator

These are two commits and we must not squash merge. git is smart an decides it's cheaper to consider this a delete + add operation instead of rename + modify

@fiji-flo Awesome ^^^ - that's my experience of git too. FWIW in the files view it is being shown as a delete/recreate, but I hope that as long as we avoid the squash it will work properly "in the end".

@wbamberg @sideshowbarker
The main reason to do wrap would be to support better editing in github. It can be a very crappy experience to edit long lines, and small changes can be hard to pick up when reviewing larger lines.

That said, I also much prefer we do not arbitrarily change the wrapping too (FWIW my personal preference tends to be line breaking on sentences - though I have not good argument for this other than it makes some manual translation software easier to use).

@wbamberg
Copy link
Collaborator

wbamberg commented Jul 16, 2021

These I believe are all the current open PRs that touch /javascript

https://github.com/mdn/content/pull/2861/files
https://github.com/mdn/content/pull/5197/files
https://github.com/mdn/content/pull/6255/files
https://github.com/mdn/content/pull/6628/files
https://github.com/mdn/content/pull/6853/files
https://github.com/mdn/content/pull/6913/files

It would be good to merge as many of these as we can in the next day or so - we already have conflicts on this PR so we'll need to redo it anyway.

I think we should decouple https://github.com/mdn/content/pull/2861/files and https://github.com/mdn/content/pull/5197/files from this PR, by converting the files in those PRs to Markdown once they are ready.

@meyerweb
Copy link
Contributor

Let us NOT hard-wrap text, ever, please and thank you.

@ddbeck
Copy link
Contributor

ddbeck commented Jul 16, 2021

I couldn't find the config to suggest against, but let's use Prettier's default "preserve" setting for prose wrapping. Then you have the option to break clauses/sentences across multiple lines, so-called semantic line breaks (I don't expect many authors to actually do this, but when you need it, you really need it).

Further reading:

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

I haven't looked at every page in detail but have looked at quite a few, and some of the more complicated ones, and they all look great.

Would it be possible to change the line-wrapping config to use "preserve", as Daniel requested in #6936 (comment) ?

Also inevitably there are conflicts now, so this will need to be regenerated anyway.

But with those changes I think we should merge this.

I'm not in on Monday, so we could aim to merge on Tuesday? We should also ask people not to merge PRs to the JS tree in between this PR being updated and it being merged.

I don't know if we want any further review from anyone, @Rumyra or @Elchi3 or anyone else in the core review crew? The previous PR: #5193 had quite a bit of scrutiny and this is almost identical (the only change I know of is mdn/yari#4221).

@Rumyra
Copy link
Collaborator

Rumyra commented Jul 19, 2021

Core js reviewers are:

Mentioning so we know to hold off on merges when told 👍

@Ryuno-Ki
Copy link
Collaborator

This PR now shows merge conflicts 😿

@wbamberg
Copy link
Collaborator

Closing in favour of #7092

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants