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

Apache License 2.0 #20

Merged
merged 4 commits into from Oct 24, 2017
Merged

Apache License 2.0 #20

merged 4 commits into from Oct 24, 2017

Conversation

bevacqua
Copy link
Contributor

We probably want at least a couple eyeballs on this PR. It's just text, but we need to make sure it doesn't sound "angry", frustrate any users or customers, etc.

@bevacqua bevacqua mentioned this pull request Oct 24, 2017
3 tasks
Copy link

@nrichers nrichers left a comment

Choose a reason for hiding this comment

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

@bevacqua I looked at this purely from a text point of view and had only minor comments. I don't think I am the right person to approve this PR. Perhaps grab someone from the legal team for approval?

LICENSE-FAQ.md Outdated
@@ -0,0 +1,25 @@
# Frequently Asked Questions

Here's our responses to questions we expect to get frequently.

Choose a reason for hiding this comment

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

"Here's" > "Here are" (responses is plural).

@@ -0,0 +1,7 @@
Copyright 2017 Elasticsearch BV

Choose a reason for hiding this comment

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

Copyright statements usually have to follow a very specific format. I think it's first year, current year plus possibly the copyright symbol, but I would check into what Elastic requires.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly just copied what Kibana does at https://github.com/elastic/kibana/blob/88f3af406dcf95828b0297372f17d417d0d2915b/LICENSE.md

I figured Kibana is doing it right

Choose a reason for hiding this comment

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

I know of at least one case where two rescue crew ended up getting thrown off the boat at speed because they had been asked to hang onto something. They hung onto each other instead of the boat. :-)

LICENSE-FAQ.md Outdated

## What is it?

Components and design directives for developing user interfaces that are on brand with Elastic.

Choose a reason for hiding this comment

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

I'd say "on brand" seems like marketing slang. Is there a better phrase you can use, like "... that reuse Elastic branding" or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with marketing slang? 😅

LICENSE-FAQ.md Outdated

Yes, but it's discouraged unless you're developing a plugin for Elastic products, or too curious. We recommend you avoid consuming `eui` entirely.

## Why is it open-source?

Choose a reason for hiding this comment

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

Global comment: use "open source" (no hyphen).

LICENSE-FAQ.md Outdated

## Can I use this?

Yes, but it's discouraged unless you're developing a plugin for Elastic products, or too curious. We recommend you avoid consuming `eui` entirely.

Choose a reason for hiding this comment

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

There has to be someone this is aimed at? If so I would state that here, rather than phrasing this text wholly negatively. A similar comment goes for line 17ff.

EDIT: This is stated in README.md, but I would repeat the following info here: "The intended consumers of this repository are [other] Elastic products."

# Elastic UI Framework

> The Elastic UI Framework is a collection of React UI components for quickly building user interfaces
> at Elastic. Not using React? No problem! You can still use the CSS behind each component.

You should check out our [living style guide][docs], which contains many examples on how components in the EUI framework look and feel, and how to use them in your products.

# Installation

Choose a reason for hiding this comment

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

As a rule of thumb, commands should have a lead-in besides just a section heading. E.g. say "To install the Elastic UI Framework:".

pugnascotia
pugnascotia previously approved these changes Oct 24, 2017
Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

Generally looks fine, bar Nik's comments.

LICENSE-FAQ.md Outdated
@@ -0,0 +1,21 @@
# Frequently Asked Questions

Choose a reason for hiding this comment

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

This file feels weird to be named "LICENSE FAQ" -- it only includes one question that is license-related and even this doesn't feel strongly suited to such a license-faq name.

Maybe put this in the README.md instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I prefer to discuss things like public interface in the README, although for a package like this which has such a large API surface, that'll probably belong on the documentation site.

I agree that the FAQ ended up not really being all that license related, what if we just renamed that file FAQ.md?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sound good to me.

pugnascotia
pugnascotia previously approved these changes Oct 24, 2017
Copy link
Contributor

@pugnascotia pugnascotia left a comment

Choose a reason for hiding this comment

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

LGTM

zinckiwi
zinckiwi previously approved these changes Oct 24, 2017
Copy link
Contributor

@zinckiwi zinckiwi left a comment

Choose a reason for hiding this comment

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

LGTM besides purely subjective comment.

FAQ.md Outdated

Here are our responses to questions we expect to get frequently.

## What is it?
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a slight preference for expanding the pronouns to “Elasic UI Framework” or “EUI” in these headings.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Added some clarity for the wording in the FAQ, making sure that stuff @epixa mentioned to me are in there. I have no commentary on whether the FAQ needs to live in the Readme or not. Some of the points there seem to be more general (like statements of purpose...etc).

FAQ.md Outdated

## What is it?

Components and design directives for developing user interfaces that are on brand with Elastic.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Elastic UI Framework (EUI) is a design library in use at Elastic to build internal products that need to share our branding and look and feel. It distributes UI React components and static assets for use in building web layouts. Alongside the React components is a Sass/CSS layer that can be used independently on its own.

FAQ.md Outdated

## Can I use this?

The intended consumers of this repository are Elastic products. You could use it when developing plugins for Elastic products such as Kibana, or if you're really curious. Otherwise, we recommend you avoid consuming `eui` entirely.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Elastic UI Framework is intended to only be used on Elastic products and is not supported as a discrete, independent design library. We do not recommend using it unless you are working on Elastic products or plugins to those products.

FAQ.md Outdated

## Why is it open source?

For a number of reasons. This code started deep in the Kibana codebase long before it made it to a standalone repository. In a sense, it was already open source, just not a first-class citizen. Other products in Elastic need to take advantage of these components, but that was hard to accomplish if the components continued to be a part of Kibana. Given Kibana is open source, this package needs to be open source. This enables us to publish on `npm` more transparently, use versioning, and maintain [an up-to-date website][docs] showcasing the functionality.
Copy link
Contributor

Choose a reason for hiding this comment

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

Because many of our products themselves are open source and rely upon this library to function. The Elastic UI Framework began as a folder of code in Kibana and we decided it could be used beyond that codebase. It exists as an independent library so that patterns can be shared across teams and design standards can be scaled across our organization. Since most of our products are open source, we treat this one similarly as far as public publishing and conversation even if its usage tends to focus more inward towards Elastic itself.

@bevacqua
Copy link
Contributor Author

@snide Your prose was great, I've switched those sections over to what you wrote

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM once legal has approved the license as it's written.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I can put together a more formal readme, but what's added in this PR seems OK to me.

@bevacqua
Copy link
Contributor Author

Baby steps.

@bevacqua bevacqua merged commit 9584c85 into master Oct 24, 2017
@bevacqua bevacqua deleted the licensing branch October 26, 2017 17:51
lcawl pushed a commit to lcawl/eui that referenced this pull request Aug 31, 2021
Add popovers section to examples
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

7 participants