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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new prop 'stateReducer' #320

Merged
merged 13 commits into from
Jan 31, 2018
Merged

Add new prop 'stateReducer' #320

merged 13 commits into from
Jan 31, 2018

Conversation

serg-plusplus
Copy link

@serg-plusplus serg-plusplus commented Jan 30, 2018

What:

At now, you could provide your own function that could modify the state that will be set. You could check the type property of stateToSet against this list to know whether you want to modify how the state is changed. @kentcdodds

Why:
Chain of ideas: #319.

How:

  internalSetState(stateToSet, cb) {
    // ...
    return this.setState(
      state => {
        state = this.getState(state)
        stateToSet = isStateToSetFunction ? stateToSet(state) : stateToSet

        // Your own function that could modify the state that will be set.
        stateToSet = this.props.modifyStateChange(stateToSet, state)
        
        // ...
      },
    // ...
  }

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

Tests and documentation I can add tomorrow(if nobody else wants).
(modify my wording, if I stumbled somewhere 馃榿 )

At now, you could provide your own function that could modify the state that will be set.
@codecov-io
Copy link

codecov-io commented Jan 30, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #320   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           4      4           
  Lines         327    329    +2     
  Branches       82     82           
=====================================
+ Hits          327    329    +2
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 e4846d8...816c8a0. 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 is an awesome start! Let's get the docs and tests done and this is totally mergible. I'm really happy with it actually! It feels like a new really useful pattern that I'll want to write a blog about and make/update a few examples with :D

kentcdodds
kentcdodds previously approved these changes Jan 31, 2018
README.md Outdated
origin of the change which has references in `Downshift.stateChangeTypes`
as in [`onStateChange`](#onstatechange).

This is a pure function. You _might_ think of it as a simple reducer!
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 where I decided to switch the function signature to accept state first. I think some folks will get giddy about this 馃槈

Copy link
Author

Choose a reason for hiding this comment

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

Right. State first its better; like simple reducer.

README.md Outdated
// selects an item with the keyboard
switch (stateToSet.type) {
case Downshift.stateChangeTypes.keyDownEnter:
case Downshift.stateChangeTypes.clickItem:
Copy link
Member

Choose a reason for hiding this comment

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

As I was copying your test over for the example I realized that if the user selected an item with the mouse it would still close and then realized we didn't have a type for that situation, so I added one in this PR 馃憤

Copy link
Author

@serg-plusplus serg-plusplus Jan 31, 2018

Choose a reason for hiding this comment

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

I know, I planned to add { type: Downshift.stateChangeTypes.itemClick } here after that PR 馃槄.
(You already did it, COOL!)

@kentcdodds
Copy link
Member

@NoTruth, could you give this a look over and make sure you like it? I'll merge it when you give me the 馃憤

Copy link
Author

@serg-plusplus serg-plusplus left a comment

Choose a reason for hiding this comment

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

I like it 馃憣! Thanks!

README.md Outdated
// selects an item with the keyboard
switch (stateToSet.type) {
case Downshift.stateChangeTypes.keyDownEnter:
case Downshift.stateChangeTypes.clickItem:
Copy link
Author

@serg-plusplus serg-plusplus Jan 31, 2018

Choose a reason for hiding this comment

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

I know, I planned to add { type: Downshift.stateChangeTypes.itemClick } here after that PR 馃槄.
(You already did it, COOL!)

@kentcdodds
Copy link
Member

Sweet. I'll merge this myself in an hour or two... I just tweeted about it and I'm sure we'll get some interesting feedback. I think maybe the prop name could be improved...

@serg-plusplus
Copy link
Author

That's a good idea thank you!

@kentcdodds
Copy link
Member

Someone suggested the name stateMiddleware. What do you think? Which is better modifyStateChange or stateMiddleware? I'm thinking that this is a pattern I want to encourage in the community. So the more general the better so there can be consensus and discoverability. That's why I'm leaning to stateMiddleware...

Though I actually kind of like stateReducer too 馃

@serg-plusplus
Copy link
Author

stateMiddleware looks good.
Also can still like stateChangeMiddleware or stateToSetMiddleware...

@kentcdodds
Copy link
Member

What about stateReducer?

@serg-plusplus
Copy link
Author

Yes, probably better reducer, because arguments looks like reducer arguments)

@kentcdodds
Copy link
Member

Cool! I think I want to change it to stateReducer. Would you like to make that update? I can if not :)

@serg-plusplus
Copy link
Author

I can forget somewhere to replace wording) 褋ould You do it?

@kentcdodds
Copy link
Member

Sure 馃憤

@geoffdavis92
Copy link
Contributor

This looks awesome. Great work @NoTruth @kentcdodds

Copy link
Author

@serg-plusplus serg-plusplus left a comment

Choose a reason for hiding this comment

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

cool, thanks

@kentcdodds kentcdodds changed the title Add new prop 'modifyStateChange' Add new prop 'stateReducer' Jan 31, 2018
kentcdodds
kentcdodds previously approved these changes Jan 31, 2018
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 ready!

@kentcdodds kentcdodds merged commit 0e9c15c into downshift-js:master Jan 31, 2018
@serg-plusplus
Copy link
Author

Awesome, thanks!

@kentcdodds
Copy link
Member

Thank you @NoTruth!

Rendez pushed a commit to Rendez/downshift that referenced this pull request Sep 30, 2018
* Add new prop 'modifyStateChange'

At now, you could provide your own function that could modify the state that will be set.

* Add simple test for modifyStateChange prop

* Update README.md

* Update downshift.js

* Update downshift.props.js

* Update downshift.js

* Update README.md

* rename to stateReducer

* cleanup readme wording

* lock version

* improve docs

* typo fix

* remove tics
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.

None yet

4 participants