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

Drawer Router #3618

Merged
merged 1 commit into from Feb 28, 2018

Conversation

Projects
None yet
5 participants
@ericvicenti
Contributor

ericvicenti commented Feb 28, 2018

Previously the drawer was a confusing configuration of two tab routers. This introduces a new and simple Drawer router

@react-navigation-ci

This comment has been minimized.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 28, 2018

Codecov Report

Merging #3618 into master will increase coverage by 0.46%.
The diff coverage is 64.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3618      +/-   ##
==========================================
+ Coverage   69.31%   69.77%   +0.46%     
==========================================
  Files          53       53              
  Lines        1711     1704       -7     
==========================================
+ Hits         1186     1189       +3     
+ Misses        525      515      -10
Impacted Files Coverage Δ
src/views/StackView/StackViewLayout.js 38.09% <ø> (ø) ⬆️
src/routers/StackRouter.js 94.76% <ø> (ø) ⬆️
src/routers/TabRouter.js 94.36% <ø> (ø) ⬆️
src/addNavigationHelpers.js 50% <0%> (-6.53%) ⬇️
src/NavigationActions.js 95.91% <100%> (+0.56%) ⬆️
src/routers/DrawerRouter.js 100% <100%> (ø)
src/navigators/DrawerNavigator.js 100% <100%> (+10%) ⬆️
src/views/Drawer/DrawerView.js 39.13% <23.07%> (-0.46%) ⬇️
src/views/Drawer/DrawerScreen.js 0% <0%> (-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 cd99dc8...562067d. Read the comment docs.

@brentvatne

This comment has been minimized.

Member

brentvatne commented Feb 28, 2018

ahhh this looks so much better!

one that may arise is that, as far as i know, there's no 'built-in' way to deep link to a screen with an open drawer. this feels like an edge case that almost nobody would care about but i've been wrong before. did you have anything in mind for how to handle that? not blocking the pr from my pov, though.

@brentvatne brentvatne merged commit 2e47cbb into master Feb 28, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@ericvicenti

This comment has been minimized.

Contributor

ericvicenti commented Feb 28, 2018

Yeah, I can see how that use case might come in handy. We could definitely augment the router's getActionForPathAndParams, so that a certain param (or path) causes the drawer to be open.

@ericvicenti ericvicenti deleted the @ericvicenti/drawer-router branch Feb 28, 2018

@browniefed

This comment has been minimized.

browniefed commented Feb 28, 2018

Once upon a time this was a thing in netflix.
If you navigated to a non-drawer item from the drawer then came back the drawer would still be open.
Not sure if this is actually possible in react-navigation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment