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 the option suppressRefError to getRootProps. #241

Merged
merged 6 commits into from
Nov 9, 2017

Conversation

yp
Copy link
Collaborator

@yp yp commented Oct 24, 2017

Fixes #235.

What:
The option suppressRefError has been added to getRootProps to bypass the ref check when it erroneously raises an error.

Why:
As discussed in #235, a ref could be correctly forwarded to the root DOM element of a composite child but the ref check could still fail.
The new option allows to bypass such a check.

How:
A new optional argument of getRootProps has been introduced.
This argument must be an object and if it has the key suppressRefError and if the corresponding value evaluates to true the ref check is bypassed (but the check that getRootProps has been called is still performed).
The introduction of the second argument of getRootProps instead of using a key in the first argument is intentional in order to clarify that it is not a prop forwarded to the root component.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged
  • Added myself to contributors table

@codecov-io
Copy link

codecov-io commented Oct 24, 2017

Codecov Report

Merging #241 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #241   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         296    299    +3     
  Branches       71     72    +1     
=====================================
+ Hits          296    299    +3
Impacted Files Coverage Δ
src/downshift.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04cc25...e199cf6. Read the comment docs.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This looks perfect! Thank you very much!

@kentcdodds
Copy link
Member

I'm going to go ahead and ask that we wait for another maintainer to review this as well 👌 thank you!

@kentcdodds
Copy link
Member

Would you like to add yourself to the contributors table?

"profile": "http://algolab.eu/pirola",
"contributions": [
"bug",
"code"
Copy link
Member

Choose a reason for hiding this comment

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

You could add "test" to this too if you want. Just update the file and commit. It'll update the readme in a commit hook 👍

kentcdodds
kentcdodds previously approved these changes Oct 24, 2017
@kentcdodds
Copy link
Member

Sorry, I'm not sure what happened with this PR! Do you mind resolving conflicts and I'll get it merged.

Easiest way to resolve all-contributors conflicts is to just fix the .all-contributorsrc by hand, then commit, the README will fix itself!

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Perfect!

@kentcdodds kentcdodds merged commit d6515be into downshift-js:master Nov 9, 2017
@kentcdodds
Copy link
Member

One day we'll have a bot so you wont have to worry about dealing with merge conflicts in the contributors table... 😅

@kentcdodds
Copy link
Member

Thanks so much for your help here and in #252! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

Rendez pushed a commit to Rendez/downshift that referenced this pull request Sep 30, 2018
…-js#241)

* Add the option `suppressRefError` to `getRootProps`.

Fixes downshift-js#235.

* Add myself as contributor.

* Fix Markdown in README.

* Add also 'test' as contribution.
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.

validateGetRootPropsCalledCorrectly does not correctly check if the root props are applied
3 participants