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 focus-trap-react library #41877

Closed
jnowakow opened this issue May 9, 2024 · 7 comments
Closed

Add focus-trap-react library #41877

jnowakow opened this issue May 9, 2024 · 7 comments
Assignees
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2

Comments

@jnowakow
Copy link
Contributor

jnowakow commented May 9, 2024

Name of library: focus-trap-react

Details

  • Link to package: https://github.com/focus-trap/focus-trap-react
  • Problem solved by using this package: It helps correctly handle focus & tab navigation in modals, RHP and LHP.
  • Number of stars in GH: 697
  • Number of monthly downloads: about 2 milion
  • Number of releases in the last year: 7
  • Level of activity in the repo: low, library is simple and only maintain
  • Alternatives: none
  • Are security concerns brought up and addressed in the library's repo?
    No security concerns
  • How many dependencies does this lib use that will be brought into our code?
    2
  • What will the effect be on the bundle size of our code?
    91.8kb

Link to PR adding the library

@jnowakow jnowakow added AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2 labels May 9, 2024
Copy link

melvin-bot bot commented May 9, 2024

Triggered auto assignment to @mountiny (AutoAssignerAppLibraryReview), see https://stackoverflowteams.com/c/expensify/questions/17737 for more details.

Copy link

melvin-bot bot commented May 9, 2024

New Library Review

  • Are all the answers in the main description filled out properly and make sense?
  • Who maintains the library and how well is it maintained?
  • How viable are the alternatives?
  • Should we build it ourselves instead?

Once these questions are answered, start a thread in #engineering-chat, ping the @app_deployers group, and call for a vote to accept the new library. Once the vote is complete, update this issue with the outcome and procede accordingly. Here is a sample post:

Hey @app_deployers,

There is a request to add a new library to App that we need to consider. Please look at this GH and then vote :+1: or :-1: on accepting this new library or not.

GH_LINK

@mountiny
Copy link
Contributor

mountiny commented May 9, 2024

@jnowakow There seems to be only one person maintaining this and the library seems to be one file. Is there a specific reason to use this library instead of just creating our own lib function that could do the same given its not that complex code?

@jnowakow
Copy link
Contributor Author

For me the reason is that it's ready to use and doesn't introduce any security concerns. It's also tested as it's quite popular. So, I don't see a point in rewriting it as it would require us to spend time on the rewrite. We can make a mistake when rewriting. And then we need to maintain the lib. In summary I don't see reason why we would need to copy it instead of using ready package

@mountiny
Copy link
Contributor

Shared here

@melvin-bot melvin-bot bot added the Overdue label May 21, 2024
@mountiny
Copy link
Contributor

@jnowakow I believe we are good to go here with the PR, we got couple 👍 and no 👎 from deployers

@melvin-bot melvin-bot bot removed the Overdue label May 21, 2024
@mountiny
Copy link
Contributor

@jnowakow this is ready to move ahead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoAssignerAppLibraryReview Auto assign someone to review a new library being added to App Weekly KSv2
Projects
None yet
Development

No branches or pull requests

2 participants