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(handlers): prevent default behavior with preventDownshiftDefault #358

Merged
merged 10 commits into from
Mar 5, 2018

Conversation

ericedem
Copy link
Collaborator

@ericedem ericedem commented Mar 5, 2018

What: allow event handlers to prevent default downshift behavior

This is the non-breaking change for #353.

Why: Previously, you could only prevent default behavior by calling event.preventDefault(). This worked, but it would also prevent events from triggering other default behaviors like form submission.

How: composeEventHandlers() now checks for event.preventDownshiftDefault in addition to event.defaultPrevented. The check for event.defaultPrevented will be removed in the next breaking change.

Checklist:

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

ericedem added 3 commits March 5, 2018 08:28
Now users can provide their own event handlers and set `event.preventDownshiftDefault = true` to completely disable the default behavior.

Also:
* Tests
* 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.

Looking awesome! Just a comment about docs.

README.md Outdated

For example:

```
Copy link
Member

Choose a reason for hiding this comment

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

Let's add javascript here so we get syntax highlighting.

Also, could we add an example that shows that you can completely override the built-in prop by adding the prop after spreading the prop getter?

Also use javascript syntax highlighting for code blocks.
@codecov-io
Copy link

codecov-io commented Mar 5, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #358   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         330    330           
  Branches       84     85    +1     
=====================================
  Hits          330    330
Impacted Files Coverage Δ
src/utils.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 10d2cf6...68277e7. 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.

Cool, looking good 👍 Just a few thoughts.

e => {
e.defaultPrevented = true
},
() => {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than this, could we use Jest mock functions (via jest.fn()) and assertions? Could make the defaultCalled cleaner as well.

README.md Outdated
},
}
```

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for the update. Could we make these examples more practical so folks who may be more beginner will understand. So like:

<input {...getInputProps()} onKeyDown={event => { /* my own stuff */} />

And the same for the example above?

src/utils.js Outdated
* They are executed in order until one of them sets
* `event.preventDownshiftDefault = true`. This allows the
* normal event to maintain its intended `event.defaultPrevented`
* state.
Copy link
Member

Choose a reason for hiding this comment

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

The last sentence of this comment is unnecessary.

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.

Thanks so much for iterating with me on this so well. We're almost there! Very well done!

README.md Outdated
## EventHandlers

You can provide your own event handlers to Downshift which will be called before the default handlers.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a simple example here:

<input {...getInputProps({onKeyDown: event => /* your handler code */ })} />

README.md Outdated
```javascript
<input
{...getInputProps({
onKeyDown(event) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's change this to an arrow function just in case someone new copy/pastes this and tries to use this and doesn't understand why it's not working.


test('call default handler when defaultDownshiftPrevented and defaultPrevented are false', () => {
const customHandler = jest.fn(() => {})
const defaultHandler = jest.fn(() => {})
Copy link
Member

Choose a reason for hiding this comment

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

If you don't care about the implementation you can just do: jest.fn() and that's the same as what you have here.

Other than that, these tests are awesome 👍

@ericedem
Copy link
Collaborator Author

ericedem commented Mar 5, 2018

Thanks so much for iterating with me on this so well.

Thanks for the feedback! Better to get this done right. :)

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 is excellent! Thank you so much for your help here!

@kentcdodds
Copy link
Member

Thanks so much for your help! 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 :)

@kentcdodds kentcdodds merged commit 07b0d95 into downshift-js:master Mar 5, 2018
@ericedem ericedem deleted the events branch March 5, 2018 22:52
kentcdodds pushed a commit that referenced this pull request Mar 5, 2018
There was an issue with a minor release, so this manual-releases.md
change is to release a new minor version. No idea what went wrong...

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

* chore(contrib): add @ericedem

* feat(handlers): prevent default behavior with preventDownshiftDefault

Now users can provide their own event handlers and set `event.preventDownshiftDefault = true` to completely disable the default behavior.

Also:
* Tests
* Docs

* docs(eventHandlers): provide override example

Also use javascript syntax highlighting for code blocks.

* refactor(composeEventHandlers): use jest for testing

* docs(eventHandlers): make examples new user friendly

* fix(composeEventHandlers): remove extraneous doc comment sentence.

* refactor(composeEventHandler): use parameter-less jest.fn()

* docs(eventHandlers): add basic example

* Update README.md


Closes downshift-js#353
Closes downshift-js#330
Rendez pushed a commit to Rendez/downshift that referenced this pull request Sep 30, 2018
There was an issue with a minor release, so this manual-releases.md
change is to release a new minor version. No idea what went wrong...

Reference: downshift-js#358
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.

3 participants