Skip to content
This repository has been archived by the owner on Nov 21, 2024. It is now read-only.

[WIP] Adding left side navigation to Layouts above 1024dp #18

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

kford55
Copy link

@kford55 kford55 commented Nov 19, 2019

Initial start to bringing the tablet and desktop layouts to Reply. This WIP PR brings the rail navigation component and an initial layout where it's added to the left side on 1024dp or greater window sizes.

@kford55 kford55 changed the title [WIP] Adding let side navigation to Layouts above 1024dp [WIP] Adding left side navigation to Layouts above 1024dp Nov 19, 2019
Copy link
Contributor

@ricknout ricknout left a comment

Choose a reason for hiding this comment

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

Looking good 👍 Although it's WIP, just one initial comment 😄

I'm getting a NPE when running on a tablet emulator and rotating twice. Seems to be the double bang on binding.bottomAppBarChevron.

Caused by: kotlin.KotlinNullPointerException
        at com.materialstudies.reply.ui.MainActivity.setUpBottomNavigationAndFab(MainActivity.kt:84)
        at com.materialstudies.reply.ui.MainActivity.onCreate(MainActivity.kt:58)

Comment on lines +71 to +72
binding.fab?.let { fab ->
fab.apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I think this could be combined into a single binding.fab?.apply { (same applies to other changes below)

Copy link
Author

Choose a reason for hiding this comment

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

Yeaa me and @hunterstich were talking about this yesterday. This as of now is a get stuff moving and started (probably gonna push the nav stuff until some of the other parts are built) but there is some changes we have to make to make this cleaner and hopefully less gross around the optionals and all that as there's definitely some lifecycle things that can cause issues here.

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