Skip to content

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

@FelixHenninger FelixHenninger force-pushed the master branch 2 times, most recently from 00b2493 to 6eee692 Compare August 18, 2020 16:33
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.

2 participants