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

Update docs on Fragment #217

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

monicao
Copy link

@monicao monicao commented Aug 1, 2017

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
Copy link

codecov bot commented Aug 1, 2017

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
Copy link

medeeiros commented Aug 4, 2017

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
Copy link
Author

monicao commented Aug 8, 2017

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
Copy link

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.

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.

2 participants