FormidableLabs / redux-little-router

A tiny router for Redux that lets the URL do the talking.
MIT License
1.04k stars 114 forks source link

Update docs on Fragment #217

Open monicao opened 7 years ago

monicao commented 7 years ago

Documents the fact that the order in which Fragments are declared matters.

Updates a small detail in another Fragments example the README to improve readability.

codecov[bot] commented 7 years ago

Codecov Report

Merging #217 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #217   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files          19       19           
  Lines         253      253           
=======================================
  Hits          249      249           
  Misses          4        4

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 6aa3297...db9ef0b. Read the comment docs.

medeeiros commented 7 years ago

This ordering is a bit weird IMHO. For example I would never declare /foo/bar before /foo, and therefore it's very error prone. Furthermore, / is also a match for both /foo/bar and /foo, so why would /foo be rendered?

My colleague Roy agrees.

monicao commented 7 years ago

The point is to explain that the order of the routes matters and that redux-little-router uses a simple routing scheme where the order of the Fragments matters.

For example I would never declare /foo/bar before /foo

If you think this will be obvious to everyone using this library, then I guess it's not worth adding it to the README. The code snippet is meant to be an example of what not to do. Perhaps that is not clear enough.

medeeiros commented 7 years ago

There is nothing wrong with this PR, it's actually very good to have it documented if that's the expected behaviour.

However, I think we should change the library to accept routes in the correct order, instead of documenting what for me is almost a bug.

Thanks for creating the PR though! I wish this was documented before I struggled trying to understand why my routes wasn't working.