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

fix some typos #2487

Merged
merged 3 commits into from
Oct 7, 2020
Merged

fix some typos #2487

merged 3 commits into from
Oct 7, 2020

Conversation

osban
Copy link
Contributor

@osban osban commented Jul 25, 2019

Found some typos. Mainly unescaped | in tables, but also a few other irregularities. Not all problems are visible in the website docs.

Found some typos. Mainly unescaped `|` in tables, but also a few other irregularities. Not all problems are visible in the website docs.
@osban osban requested a review from dead-claudia as a code owner July 25, 2019 00:01
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

Like the idea, but it needs a few edits.

docs/route.md Outdated Show resolved Hide resolved
docs/route.md Outdated Show resolved Hide resolved
docs/route.md Outdated Show resolved Hide resolved
docs/route.md Outdated Show resolved Hide resolved
docs/route.md Outdated Show resolved Hide resolved
Copy link
Member

@dead-claudia dead-claudia left a comment

Choose a reason for hiding this comment

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

(Hit the wrong button...)

@dead-claudia
Copy link
Member

I do want to note that it renders correctly, so I do want to see it rendered - I don't want a \| literally in the site's output. (The website is higher-priority than GitHub.)

@dead-claudia dead-claudia added the Area: Documentation For anything dealing mainly with the documentation itself label Jul 26, 2019
@osban
Copy link
Contributor Author

osban commented Jul 26, 2019

What do you prefer? I can remove the table changes too, and if not, is there a way to check how it actually renders on the site?

@dead-claudia
Copy link
Member

@osban

What do you prefer?

I'd prefer your patch + my requested edits if it generates correctly, because that lets GitHub and the website both have correct formatting.

I can remove the table changes too, and if not, is there a way to check how it actually renders on the site?

  1. npm install http-server -g if you don't already have it installed locally.
  2. Run npm run gendocs.
  3. Run http-server ./dist to start the web server. It'll open on port 8080 by default, but you can change it via -p PORT.
  4. Open a web page to http://localhost:8080/route.html and look at the result.
  5. Hit Ctrl+C when you're done.

It's slightly involved, but I've done similar so many times it's almost brainless at this point. And yes, http-server is an absolute life saver here. If you want it running concurrently so you can just refresh and see changes immediately, just open a different terminal to your local repo, run http-server -c-1 (the -c -1 disables caching), run npm run gendocs when you need to update the docs, and open the developer tools. (Just make sure that you at some point have caching disabled in the network tab or equivalent.)

And of course, you can make this a little more magical with chokidar-cli "docs/**" -c "npm run gendocs". I don't usually go that far before writing custom local scripts, though.

@dead-claudia
Copy link
Member

I probably should at some point write a utility wrapping the two - I find myself doing this quite a bit in various places. It's nice when dealing with projects with almost no real build system involved, just scripts everywhere.

@osban
Copy link
Contributor Author

osban commented Jul 27, 2019

Thanks, it works.

Now, to avoid misunderstandings, when you say that it renders correctly on the website, do you mean that Object<String,Component|RouteResolver> should render as Object (as it does now), or as Object<String,Component|RouteResolver>?

@dead-claudia
Copy link
Member

dead-claudia commented Jul 27, 2019

@osban As Object<String,Component|RouteResolver>. And it currently renders as this. Edit: On the website, I mean. It's why I linked to it in the first place, so you could see it in the signature.

BTW, you might want to update your branch - I made a few quality of life improvements to the build scripts. It also enables certain docs lints. There's one major thing you'll appreciate: it has a npm run watch:docs built-in. So you just need to run http-server and npm run watch:docs in parallel. In *nix, it's as easy as http-server -c-1 & npm run watch:docs, and in Windows batch, you can do start /b http-server -c-1 & npm run watch:docs to similar effect (start is what makes it parallel - & is basically Bash's ;).

@osban
Copy link
Contributor Author

osban commented Jul 27, 2019

Uhm, are we looking at the same line? I'm seeing Object at the routes argument at the signature (I've checked it in Chrome, Firefox, and Edge).

I'll look at the upgrade tomorrow :)

@dead-claudia
Copy link
Member

@osban Better link: https://mithril.js.org/hyperscript.html#signature

Those | are what I'm referring to. Sorry if it wasn't clear.

@osban
Copy link
Contributor Author

osban commented Jul 28, 2019

No worries, I will make sure route.md will render correctly on both the website and Github. It turns out jumping through hoops is sometimes necessary 😕

Btw, is there an easy way to update my branch?

@osban
Copy link
Contributor Author

osban commented Jul 28, 2019

About the script (in Windows):

  • & doesn't work in Powershell, but does work in cmd
  • it look like start /b http-server -c-1 & start /b npm run watch:docs is needed to run them both in the same window
  • I'm getting an error on the script:
internal/util.js:209
    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'original', 'function');
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "original" argument must be of type function
    at promisify (internal/util.js:209:11)
    at Object.<anonymous> (C:\Users\Oscar\Documents\GitHub\mithril.js\scripts\generate-docs.js:6:18)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Module.require (module.js:596:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (C:\Users\Oscar\Documents\GitHub\mithril.js\scripts\watch-docs.js:5:18)

@osban
Copy link
Contributor Author

osban commented Jul 28, 2019

What is the best way to get the new changes committed? I made this commit online, but I now have a local repo. Shall I amend this commit or PR (not sure how to do that) or create a new one (should that be on a new branch?)?

@dead-claudia
Copy link
Member

@osban

Btw, is there an easy way to update my branch?

Try git pull --rebase upstream next (change upstream to whatever your upstream is configured to)

it look like start /b http-server -c-1 & start /b npm run watch:docs is needed to run them both in the same window

Okay, I may have been incorrect on that, then. My bad - I don't use Windows hardly ever, and it's been years since I last regularly used it.

& doesn't work in Powershell, but does work in cmd

Why I said "Windows batch". 😉 Powershell uses ; instead of &, but you probably already knew that.

I'm getting an error on the script:

Upgrade to the latest stable Node. It should work there, and that's what CI uses. (stream.pipeline was added in Node 10.)

If you need to switch between this and an older version, something like nvm but for Windows (like nvm-windows or nodist) could help.

@dead-claudia
Copy link
Member

What is the best way to get the new changes committed? I made this commit online, but I now have a local repo. Shall I amend this commit or PR (not sure how to do that) or create a new one (should that be on a new branch?)?

Go to the patch-2 branch in your repo, and navigate to the file you need to modify, and edit it the usual way. Just make sure you commit directly to that branch instead of creating a PR (it's an option below the editor before you click to commit, but IIRC it's direct by default except on the main branch). I wish GH would be a bit more intuitive on that front, but it isn't.

Fix those symbols inside a Markdown table, so they render correctly on the website and Github.
@osban
Copy link
Contributor Author

osban commented Jul 29, 2019

Aha, thanks for the explanation. The whole Git thing is very confusing to me, though I'm slowly starting to understand it a little better. I've updated (read: completely overhauled) the original changes.

dead-claudia
dead-claudia previously approved these changes Jul 29, 2019
@dead-claudia
Copy link
Member

@osban Just to clarify, does this render correctly to the site also? You can just compile the docs and inspect the HTML output in dist/ pretty easily.

@osban
Copy link
Contributor Author

osban commented Jul 29, 2019

@isiahmeadows Yeah, I checked it using the 'old' gendocs 😉

@dead-claudia
Copy link
Member

dead-claudia commented Jul 30, 2019

@osban Re-sync your branch with next - I fixed the file descriptor leak. @porsager reported errors of an unclosed HandleScope in C++-land in #2348, but I'm not getting those locally. If you do happen to get those, DM me on either Gitter or on Twitter, because I want to look into it. It's clearly a Node bug (probably a race condition if I had to guess), but I'd like to maybe see if I can get a repro going on my end.

@osban
Copy link
Contributor Author

osban commented Aug 3, 2019

@isiahmeadows I have pulled the latest version, upgraded Node to v10, did npm rebuild. When running either npm run watch:docs or npm run build:docs I'm getting:

fatal: invalid reference: gh-pages
Error: Command failed: git checkout gh-pages -- archive
fatal: invalid reference: gh-pages

    at checkExecSyncError (child_process.js:629:11)
    at execFileSync (child_process.js:647:13)
    at generate (C:\Users\Oscar\Documents\GitHub\mithril\scripts\generate-docs.js:57:2)
fatal: invalid reference: gh-pages

@porsager
Copy link
Contributor

porsager commented Aug 3, 2019 via email

@osban
Copy link
Contributor Author

osban commented Aug 3, 2019

@porsager thanks! For now I'm just using the old generate.js to build the docs...no watching yet, but oh well 😉

@dead-claudia
Copy link
Member

@osban @porsager Yeah, I'd say comment them out for now and if you could, file a bug so I know to fetch it to ensure it actually exists. Sorry for the complications! (It works for me locally, but I have a near-full clone of Mithril's repo. 🙃)

@dead-claudia dead-claudia merged commit 3ad4040 into MithrilJS:next Oct 7, 2020
@dead-claudia
Copy link
Member

Fixed the small conflict and merged.

@JAForbes JAForbes mentioned this pull request Apr 28, 2022
StephanHoyer pushed a commit that referenced this pull request May 16, 2022
* fix some typos

Found some typos. Mainly unescaped `|` in tables, but also a few other irregularities. Not all problems are visible in the website docs.

* fix `<>` and `|` rendering

Fix those symbols inside a Markdown table, so they render correctly on the website and Github.

Co-authored-by: Isiah Meadows <contact@isiahmeadows.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation For anything dealing mainly with the documentation itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants