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

feat: add useStateRef hook #1113

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

michaltarasiuk
Copy link
Contributor

@michaltarasiuk michaltarasiuk commented Jan 28, 2023

Hook description

One rudimentary way to measure the position or size of a DOM node is to use a callback ref. React will call that callback whenever the ref gets attached to a different node.

Note that we pass [] as a dependency array to useCallback. This ensures that our ref callback doesn’t change between the re-renders, and so React won’t call it unnecessarily, so in current example the callback ref will be called only when the component mounts and unmount. As in result we will not fall into infinite re-renders.

Checklist

  • Have you read the contribution guidelines?
  • If you are porting a hook from react-use, have you checked Port remaining hooks from react-use #33 and the migration guide
    to confirm that the hook has been approved for porting?
  • Does the code have comments in hard-to-understand areas?
  • Is there an existing issue for this PR?
    • link issue here
  • Have the files been linted and formatted?
  • Have the docs been updated?
  • Have you written tests for the new hook?
  • Have you run the tests locally to confirm they pass?

@codecov
Copy link

codecov bot commented Jan 28, 2023

Codecov Report

Merging #1113 (f833f4a) into master (f6829c0) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1113      +/-   ##
==========================================
+ Coverage   98.52%   98.53%   +0.01%     
==========================================
  Files          63       64       +1     
  Lines        1082     1091       +9     
  Branches      180      180              
==========================================
+ Hits         1066     1075       +9     
  Misses          2        2              
  Partials       14       14              
Impacted Files Coverage Δ
src/index.ts 100.00% <100.00%> (ø)
src/useStateRef/index.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xobotyi
Copy link
Contributor

xobotyi commented Jan 28, 2023

So, you've

  • added new dependency (not compatible with our testing pipeline
  • globally disabled linting rule without any reasoning
  • once again ignored PR guidelines

Last one is somehow "forgivable"

But first two are no-go since they should be separate pull requests with reasoning

@michaltarasiuk
Copy link
Contributor Author

Hi @xobotyi, in days I will create a pull request for two first points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants