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

Add installation instructions to the README #1088

Closed
wants to merge 7 commits into from
Closed

Add installation instructions to the README #1088

wants to merge 7 commits into from

Conversation

nilslindemann
Copy link

@nilslindemann nilslindemann commented Nov 30, 2023

Feel free to modify as you desire, @paldepind

Explain how to install this thing, especially the examples.
@nilslindemann nilslindemann mentioned this pull request Nov 30, 2023
@nilslindemann
Copy link
Author

I do not understand the failing test. This is just an edit in the README.

@iambumblehead
Copy link
Contributor

the browser test pipeline fails intermittently and frequently on first-push

❌ The browser was unable to create and start a test page after 30000ms. You can increase this timeout with the browserStartTimeout option. (failed on firefox)

if you push up any sort of small change to trigger the pipeline it usually passes on the second try

@paldepind
Copy link
Member

I think these instructions are great. But I'm not sure about how best to include them in the readme. To me at least, an "installation" section means what users have to do to install a library in their own project.

Perhaps this section could be included in the CONTRIBUTING.md? That would make sense if we think the instructions are for people who want to contribute. But, these instructions are also useful for people who just want to play around with the example. So we could also include the instructions as a "How to run the examples" section. What do you think @nilslindemann?

@nilslindemann
Copy link
Author

Thanks, @iambumblehead, that's an unusual pipeline. But it was a good fail, because I found a few other things to fix.

@nilslindemann
Copy link
Author

@paldepind

rename section to "How to run the examples"

That is a good point. See my commit "Rename and move installing instructions". In that new context, I also moved the instructions below the section "More examples". Let me know if there are issues.

Add "installation" section to CONTRIBUTING.md which also explains what users have to do to install a library in their own project, and for people who want to contribute.

That is also an excellent idea. I do not know how to install snabbdom as a library, so I would like to ask someone more experienced than me to do that :-)

@nilslindemann
Copy link
Author

Sorry, I saw too late that you use a specific commit format.

README.md Outdated Show resolved Hide resolved
According to kuraga's suggestions
@nilslindemann
Copy link
Author

nilslindemann commented Dec 1, 2023

I would be very interested, as paldepind already mentioned, to also add a paragraph, how to add snabbdom as a project dependency. I just tried to do that by adding it to the dev dependencies in package.json but failed. Could someone help out here?

Also, I think we could then move that section as simply "Installation" below the TOC (or into CONTRIBUTING.md and point to it, I do not mind where it is, as long as it is somewhere)

@kuraga
Copy link

kuraga commented Dec 1, 2023

@nilslindemann , npm install snabbdom inside the project's root. Or, add snabbdom in package.json, then npm install.

README.md Outdated

```bash
npm test
````
Copy link

Choose a reason for hiding this comment

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

There is one redundant tilde.

README.md Outdated
```bash
git clone https://github.com/snabbdom/snabbdom # clone the snabbdom repository
cd snabbdom
npm install # install the requirements
Copy link

Choose a reason for hiding this comment

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

Suggested change
npm install # install the requirements
npm install # install the dependencies

- [Example](#example)
- [More examples](#more-examples)
- [How to run the examples and tests locally](#how-to-run-the-examples-and-tests-locally)
- [Table of contents](#table-of-contents)
Copy link

Choose a reason for hiding this comment

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

Suggested change
- [Table of contents](#table-of-contents)

README.md Outdated
@@ -118,16 +118,48 @@ patch(vnode, newVnode); // Snabbdom efficiently updates the old view to the new
- [Hero transitions](http://snabbdom.github.io/snabbdom/examples/hero/)
- [SVG Carousel](http://snabbdom.github.io/snabbdom/examples/carousel-svg/)

### How to run the examples and tests locally
Copy link

Choose a reason for hiding this comment

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

Suggested change
### How to run the examples and tests locally
## How to run the examples and the tests

README.md Outdated
- [Features](#features)
- [Example](#example)
- [More examples](#more-examples)
- [How to run the examples and tests locally](#how-to-run-the-examples-and-tests-locally)
Copy link

Choose a reason for hiding this comment

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

Move left?

@kuraga
Copy link

kuraga commented Dec 1, 2023

Also, I think we could then move that section as simply "Installation" below the TOC (or into CONTRIBUTING.md and point to it, I do not mind where it is, as long as it is somewhere)

Add the ## Installation section (one of the first ones).

@nilslindemann
Copy link
Author

nilslindemann commented Dec 1, 2023

@kuraga

npm install snabbdom inside the project's root. Or, add snabbdom in package.json, then npm install.

Ok, it is a few months since I did this last and I forgot a few things, it seems.

This is what I did:

Create a folder snabbdom-test

cd snabbdom-test
npm init
npm install snabbdom

Create a file examples/index.html1 with the contents:

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <title>snabbdom Test</title>
</head>

<body>
    <div id="container"></div>
    <script src="examples/logic.js"></script>
</body>

</html>

Create a file examples/logic.js [1] having the example given in the README as content.

Run npx serve. Go on http://localhost:3000/examples.

Now the browser console gives Uncaught SyntaxError: Cannot use import statement outside a module (at logic.js:1:1) This is the point where I forgot what I need to do. Can you give me a hint, @kuraga ?

1 in a folder examples, because npx serve redirects to this folder. Dunno why it does that.

@kuraga
Copy link

kuraga commented Dec 1, 2023

@nilslindemann , well. "install to get the snabbdom module importable" (in the server script) and "install to get the Snabbdom working in the browser" are different intentions :)

Now, you're able to run something like the Example in nodejs REPL.

But yeah, a good point to mention.

To others: Rollup, Webpack?

@nilslindemann
Copy link
Author

So, @kuraga, I would love to have this documented. We should do that.

  • What could be a minimal node.js helloworld repl example which does something useful, but does it with snabbdom technology?
  • Minimal browser example with webpack/Rollup?

I will for the moment just commit your above fixes but leave the chapter where it currently is.

@kuraga
Copy link

kuraga commented Dec 1, 2023

@nilslindemann , formally,

What could be a minimal node.js helloworld repl example which does something useful, but does it with snabbdom technology?

The Example's code.

Minimal browser example

Like in the examples/* directories with some commands from npm run build.

As for

Minimal browser example with webpack/Rollup?

So, I would love to have this documented. We should do that.

Yes. Later.

And maybe it's out of scope.

@nilslindemann
Copy link
Author

No. Not later. Now or never. So never.

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

Successfully merging this pull request may close these issues.

None yet

4 participants