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 and add npm callout to README #81

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jdpigeon
Copy link
Contributor

No description provided.

@FelixHenninger
Copy link
Owner

Hi Dano, many thanks for your PR! 😍 I'd just have merged it directly, but you got me thinking: I'd have maintained that the builder makes things easy, but then again I got whacked in the review process recently for making this kind of claim 😉

I had two ideas going through your diff:

  • Do you think it might be worth linking to the package page on NPM?
  • Would it make sense to provide some context around the npm install command? I was wondering whether folks who need the pointer to npm install might need some further information, whereas pro users might not need either.

I'd be happy to add either (though of course you're also welcome to), but for sure I'd appreciate your thoughts.

(also, PRs now pass CI! 🎉 This makes me very happy indeed)

@jdpigeon
Copy link
Contributor Author

Awesome, great news that we got CI up and running!

I agree and think the README could be a little more clear. I just pushed another commit that breaks up the different strategies to getting started. Let me know what you think.

It also might be helpful to put up a CDN link. I think you mentioned that lab.js was available on a CDN somewhere? I guess since we're on npm, we're also on unpkg

@FelixHenninger
Copy link
Owner

Hej Dano, thanks, that looks great!

Your comment about the CDN link got me thinking, and I discovered that I'd collected some of this info on the library readme in the corresponding subdirectory, years ago now 😅🤦. Maybe it's time to merge the two documents?

Thanks again for your effort and support!

@FelixHenninger
Copy link
Owner

Hi Dano, would you mind setting your PR to editable, or would you mind me pulling it in and adding some minor edits? I think this is absolutely worth merging, and I'm happy to fix the points I've been nitpicking about.

Best, -Felix

Base automatically changed from master to main January 19, 2021 13:35
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

2 participants