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 missing 'suppressRefError' option typings #556

Merged

Conversation

rifler
Copy link
Contributor

@rifler rifler commented Aug 20, 2018

What:
Missing TypeScript (TS) typings added - second argument to getRootProps and getMenuProps functions

Checklist:

  • Documentation
  • Tests N/A
  • Ready to be merged
  • Added myself to contributors table

@rifler rifler force-pushed the rifler.suppress-ref-error-typings branch from 66c2c4a to a2233a7 Compare August 20, 2018 12:39
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.

Changes look great except those in the README. Could you remove that? Thanks!

README.md Outdated
@@ -495,6 +496,12 @@ node you render in downshift). Internally we use
so if you use that package then you wont be adding any additional bytes to your
bundle :)

### suppressRefError
Copy link
Member

Choose a reason for hiding this comment

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

This is excluded from the docs intentionally. It's a pretty far edge case so I don't think it's worth including in the docs.

@rifler rifler force-pushed the rifler.suppress-ref-error-typings branch from a2233a7 to e8eebe6 Compare August 20, 2018 17:02
@rifler
Copy link
Contributor Author

rifler commented Aug 20, 2018

fixes done
I just noticed that there is very similar PR - #534
They overlap a bit

@kentcdodds
Copy link
Member

Thanks! @rifler, could you review #534? Then I can merge it and you can update this one to fill in any gaps?

@rifler
Copy link
Contributor Author

rifler commented Aug 20, 2018

@kentcdodds LGTM, I'll rebase my PR after merging #534

@rifler rifler force-pushed the rifler.suppress-ref-error-typings branch 3 times, most recently from 627c4a8 to 2c74580 Compare August 21, 2018 04:28
@rifler
Copy link
Contributor Author

rifler commented Aug 21, 2018

done
Do you run prettier/linters on precommit? It's a bit hard to manage semicolons manually

@kentcdodds
Copy link
Member

Yes... Do you have dependencies installed?

@rifler
Copy link
Contributor Author

rifler commented Aug 21, 2018

haha, it was so obvious, sorry. I had an experience to run prettier/linters (without --fix) also on CI, I think it is more reliable solution than pre-commit

@kentcdodds
Copy link
Member

I'm not sure why the precommit script is not running for you. It should be automatically generating the contributors table in the README as well as running prettier on all changed files... 🤔

@rifler
Copy link
Contributor Author

rifler commented Aug 22, 2018

It's because I didn't install npm deps, as you mentioned earlier. It was obvious, but I didn't catch it :) Now it is ok.
About CI I meant the following - CI checks are more reliable, e.g. if someone (like me) forget to install deps (or use --no-verify), than CI fails
For example my PR contains style fixes (semi removal) from previous contributor
https://github.com/paypal/downshift/pull/556/files#diff-494415c3af35245ecae73f5d42d8b331R117
I suppose it is because he forgets to install deps too.
Also we could avoid errors like this - 5acaf86

@rifler rifler force-pushed the rifler.suppress-ref-error-typings branch from 2c74580 to b1c1334 Compare August 22, 2018 07:54
@kentcdodds
Copy link
Member

How do you suggest we update the pull request someone else opened? I know it's possible, but it'd be a bit more work than I would like to do myself.

@kentcdodds kentcdodds merged commit d46794c into downshift-js:master Aug 22, 2018
@kentcdodds
Copy link
Member

🎉 This PR is included in version 2.1.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@rifler rifler deleted the rifler.suppress-ref-error-typings branch August 23, 2018 08:07
@rifler
Copy link
Contributor Author

rifler commented Aug 23, 2018

On CI we don't modify code, just fail check. After that author and reviewer realise that something went wrong with precommit hook and fix it

Summarize:

  1. Pre-commit: we use lint-staged, lint and fix only changed files;
  2. CI on pr change: lint all files (not fix!), fail check on error.

We ran into same problems with linters, because colleagues forget to install deps or pass --no-verify to git or other reasons.

Nevertheless, thanks for your quick reaction for this PR!

@kentcdodds
Copy link
Member

Oh, we definitely do that via the validate script. I'm not sure why that last one got through. I probably merged it before the build was finished or ignored the failing build because we'd been having intermittent build failures anyway. Either way, it was my fault 😅

Rendez pushed a commit to Rendez/downshift that referenced this pull request Sep 30, 2018
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.

2 participants