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

Add rule preventing importing certain components from react-native #38

Merged
merged 5 commits into from
Dec 3, 2021

Conversation

puneetlath
Copy link
Contributor

@puneetlath puneetlath commented Dec 2, 2021

In this issue/PR I have renamed our custom Text and Button components to ExpensifyText and ExpensifyButton. We want contributors to use these custom components rather than using the default Text and Button components from react-native.

This rule will throw a lint error when either of those two components is imported from React Native. That looks like the following:
image

The rule can be suppressed by adding a comment like // eslint-disable-next-line no-restricted-imports.

I have also updated the Readme to say that the App repo needs to be updated after the ESLint configuration is updated, not just Web and Web-Secure.

And finally, I have a list of follow-up todos once this PR is merged here: https://github.com/Expensify/Expensify/issues/187203

@puneetlath puneetlath self-assigned this Dec 2, 2021
@puneetlath puneetlath marked this pull request as ready for review December 2, 2021 16:55
@puneetlath
Copy link
Contributor Author

puneetlath commented Dec 3, 2021

@marcaaron it seems there is no puller bear in this repo and Github suggested I assign you. You have some context on this so I did, but let me know if there's a different way I should go about assigning!

rules/expensify.js Outdated Show resolved Hide resolved
@puneetlath
Copy link
Contributor Author

puneetlath commented Dec 3, 2021

One of our contributors recommended we add TextInput as well, since we have a handful of our own custom TextInput components that we want people to use. I think it's a good idea.

I went ahead and added that @marcaaron but can revert if you disagree.

@marcaaron
Copy link
Contributor

I like that idea but see we have a few TextInput that should maybe be converted to ExpensiTextInput if possible. Took a look and we still have:

TextInputWithFocusStyles, TextInputWithLabel, TextInputAutoWidth

So it's really confusing which one we should be using...

Maybe we can create an issue to:

  1. Figure out how to replace or standardize these into ExpensiTextInput
  2. Update all the usages
  3. Then block importing TextInput

Or we can leave what you have and try to do that cleanup in another PR / issue. Either way works.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

LGTM - feel free to merge @puneetlath or update to remove the TextInput if you agree we should make those usages more consistent before getting strict here. I don't have strong feelings on this tho so approving.

@puneetlath
Copy link
Contributor Author

It looks like the only files importing react native TextInput right now are our our various custom text input components. So I think we're fine to leave the rule in place. But I also like the idea of cleaning up these various inputs to make it more intuitive. I'll create a follow-up issue for that. Thanks!

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

2 participants