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

Remove unused Chart.js dependency #824

Merged
merged 1 commit into from Jan 7, 2021
Merged

Conversation

benmccann
Copy link
Contributor

This dependency didn't appear to be used anywhere

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2021

CLA assistant check
All committers have signed the CLA.

@benmccann
Copy link
Contributor Author

I'm generally happy to sign CLAs. There's some unclear language here and I'm wondering if it could be updated before I sign.

"our open source software projects" - this is a bit vague. Can it be changed to say PhotoPrism or something more specific?
"with regard to the Projects in respect of any of the Projects" - this isn't quite an understandable sentence.

Here's a sample CLA from another project if it helps determine some clearer language: https://cla-assistant.io/ampproject/amphtml?pullRequest=31656

@lastzero
Copy link
Member

lastzero commented Jan 7, 2021

I'm generally happy to sign CLAs. There's some unclear language here and I'm wondering if it could be updated before I sign.

"our open source software projects" - this is a bit vague. Can it be changed to say PhotoPrism or something more specific?
"with regard to the Projects in respect of any of the Projects" - this isn't quite an understandable sentence.

This is in fact a standard CLA (e.g. also used by SAP) that was adapted and verified by our lawyer. I'm not a lawyer and I don't feel it's right to change the terms on my own after getting up in the morning, even though we'd love to merge this.

We have multiple repos in our organization and want to avoid the need for multiple CLAs for every single repo. I think that is quite understandable. It doesn't mean all open-source projects on GitHub belong to us.

@lastzero lastzero added the waiting Impediment / blocked / waiting label Jan 7, 2021
@benmccann
Copy link
Contributor Author

Here's SAP's CLA. Neither of the two portions I quoted appear in their CLA, so it doesn't seem like it's actually a standard text. https://gist.github.com/CLAassistant/bd1ea8ec8aa0357414e8

That being said I'll go ahead and sign with the understanding that this refers only to code under the photoprism GitHub organization

@lastzero
Copy link
Member

lastzero commented Jan 7, 2021

Thank you! Of course it only refers to this organization and specifically to repos where the CLA link / button appears. We've switched our Geolocation API to private a while ago, as we prefer to develop it without contributors at the moment - writing docs for and maintaining photoprism/photoprism is more than enough work for us right now. That might change again in the future. No worries, we won't sue you! :)

Standard text or not, we don't have the money to consult a lawyer every time a change request or question comes up. I can't recall all the details how we came up with this version, all I know is that it was edited and approved by our lawyer. Note that US and German laws are not the same, so a standard CLA might not be good for us here in Germany.

Thank you very much for your contributions so far, lots of valuable input 🥳

@benmccann
Copy link
Contributor Author

Sounds good. Is there anything else we need to merge this PR?

@lastzero
Copy link
Member

lastzero commented Jan 7, 2021

I'm starting to go through PRs now 👍 We typically don't get that many, hope I'll get all merged today!

@lastzero
Copy link
Member

lastzero commented Jan 7, 2021

Re Chart.js: We wanted to build a dashboard with it, but prioritized other, more important features instead.

@lastzero lastzero merged commit ba11d87 into photoprism:develop Jan 7, 2021
@lastzero lastzero added released Available in the stable release and removed waiting Impediment / blocked / waiting labels Jan 7, 2021
@benmccann
Copy link
Contributor Author

Cool. I'm actually one of the core maintainers of Chart.js, so let me know if you ever have any questions about it 😄 We're working on a 3.0 release, which should be a lot faster to render charts!

@benmccann benmccann deleted the chart.js branch January 7, 2021 17:17
@lastzero
Copy link
Member

lastzero commented Jan 8, 2021

Cool. I'm actually one of the core maintainers of Chart.js, so let me know if you ever have any questions about it

Thank you! Noticed that already 😉

Justin, Brian and I have been working on JavaScriptMVC, starting around 2007. From what I know, it was the first (open-source) JS MVC framework - thus the funny name.

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

Successfully merging this pull request may close these issues.

None yet

3 participants