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

feat: Open npm url in new tab. Add link title. #597

Closed
wants to merge 4 commits into from

Conversation

webmasterkai
Copy link
Contributor

As per mvader

@tkurki
Copy link
Member

tkurki commented Aug 23, 2018

Maybe squash the doc related commits?

Was there something specific that prompted the sass update?

@webmasterkai
Copy link
Contributor Author

webmasterkai commented Aug 23, 2018

Yeah, it was failing to compile on my machine. The version bump was mostly due to requiring node 5 or later. I thought all the commits got squashed on merge. Is it better that I do it now? Is it to make it easier to review?
[edit] Or maybe it would be better to have a docs commit and src code change commit... Maybe two different pull requests?

@tkurki
Copy link
Member

tkurki commented Aug 24, 2018

In github you can do merge commit, squash them all or just add the commits to master. Depends. 2 PRs would have been optimal here, as the commits are not related, but combining the doc updates to one is ok.

I did not quite understand requiring Node 5?

@webmasterkai
Copy link
Contributor Author

The compile process wasn't working on my machine. Said it wasn't yet compatible with my architecture. I tried updating sass-loader to the latest but the 6x-7x has breaking changes:

  • Drop official node 4 support
  • This slightly changes the resolving algorithm. Should not break in normal usage.
  • Throws an error at runtime now and refuses to compile if the peer dependency is wrong.

@tkurki
Copy link
Member

tkurki commented Aug 26, 2018

Doc added in #599. Npm links opening in #600 and published as @signalk/[email protected].

Don't want to move ahead with Sass upgrade, it works on my machine after node_modules wipe and is a sidetrack to this PR, so closing.

Thank you!

@tkurki tkurki closed this Aug 26, 2018
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