Skip to content
This repository has been archived by the owner on Oct 26, 2018. It is now read-only.

Add a CHANGELOG and update the README #90

Closed
wants to merge 2 commits into from

Conversation

geowarin
Copy link
Contributor

@geowarin geowarin commented Dec 7, 2015

Hi guys,

I just wanted to give a hand with #84.
I realise it is not perfect so feel free to use it as a base or make a better PR, I won't be offended 😃


### `updatePath(path, noRouterUpdate)`
### `pushPath(path, state = {}, avoidRouterUpdate = false)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not match the api: https://github.com/jlongster/redux-simple-router/blob/3545a3e7fea0d8629180c774e16825d0d4ce547c/src/index.js#L23 (e.g. state does not have a default value)

I actually think you can just copy the line from the code.

@ellbee
Copy link
Member

ellbee commented Dec 7, 2015

Very important: Just look at this code: the whole thing is only 68 lines of JS. We need a recount here 😉

@ellbee
Copy link
Member

ellbee commented Dec 8, 2015

The piece of state is just a URL string, whereas redux-router stores the full location object from react-router.

The state is a little bit more complicated now. Maybe this should say something like:

The piece of state of simpler. redux-router stores the entire location object from react-router

@justingreenberg
Copy link
Contributor

@geowarin please see #89 #93 i'm sorry i had included the README updates in previous PR and was asked to split out, had no idea you already tackled this

@ellbee i had reworded it to "We only store current URL and state, whereas redux-router stores the entire location object from react-router." Once we support search and query keys we can say we support the whole object. In reality this should be history, not react-router, but I left it as-is for simplicity

@geowarin
Copy link
Contributor Author

geowarin commented Dec 8, 2015

@justingreenberg No problem, #93 is a much better PR. Should I close this one?
Maybe you could add the changelog to your PR also?

@justingreenberg
Copy link
Contributor

@geowarin i added the changelog stuff to #93—thank you again 😄

@geowarin geowarin closed this Dec 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants