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 missing Relay import in examples #453

Closed
wants to merge 1 commit into from

Conversation

kassens
Copy link
Member

@kassens kassens commented Oct 12, 2015

Test Plan:
For every example npm install and npm start then open it in a browser and
see it render.

Test Plan:
For every example `npm install` and `npm start` then open it in a browser and
see it render.
@kassens
Copy link
Member Author

kassens commented Oct 12, 2015

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/393582277519735/int_phab to review.

wincent referenced this pull request Oct 12, 2015
Summary: 1. Fixed the licence of examples. Now it is same as define [here](https://github.com/facebook/relay/tree/master/examples/todo#license).
2. Fixed some lint errors and warnings.

Ps - All npm test are passed and lint shows no major errors. Please see the diff.

Npm Test
<img src="https://box.everhelper.me/attachment/301722/NuD6t5ei9i0VAGxX7avOK5Ifa4bkQt12/312416-0JwJzhXkXu4ZATUS/screen.png"/>

Lint test
<img src="https://box.everhelper.me/attachment/301723/pGqakY01HpgeAsKPfGOtus6xDsK7NCIc/312416-AJM6V5LS06uq0P5c/screen.png"/>
Closes #433

Reviewed By: @wincent

Differential Revision: D2514888

fb-gh-sync-id: c9defaf7b7694ebba4a7017be07ff6ae56dbe9dd
@akashnimare
Copy link
Contributor

Dude I removed those relay import line because it is unused var. Lint was showing that var relay is not used so I removed it.

@yungsters
Copy link
Contributor

cc @zpao @spicyj

I assume this is an implicit dependency due to JSX? Is this something we could fix in eslint-plugin-react?

@sophiebits
Copy link
Contributor

Yeah, you're supposed to import React. I guess that could be a nice lint rule, though in some cases React will be a global so you wouldn't want to warn.

@yungsters
Copy link
Contributor

though in some cases React will be a global so you wouldn't want to warn.

But in that case, we should use the existing ESLint mechanisms for declaring React as a global, right?


Regardless, @kassens 👍

@zpao
Copy link
Member

zpao commented Oct 13, 2015

@sophiebits
Copy link
Contributor

@yungsters
Copy link
Contributor

Ahh, nice.

@kassens Can you incorporate @zpao's suggestion so that this pull request does not re-introduce the lint errors?

@kassens kassens closed this in 2b0ba4a Oct 13, 2015
@kassens kassens deleted the missing-react branch October 13, 2015 18:52
kassens added a commit that referenced this pull request Oct 14, 2015
Summary: This fixes a few lint issues and also uses a single `.eslintrc` to prevent duplication and divergence of lint rules.

Follow up for concerns raised in #453.
Closes #460

Reviewed By: @yungsters

Differential Revision: D2537193

fb-gh-sync-id: 116a852c0f75bb0ea2d657f862db6c211622632d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants