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

Detail page has two toolbars #75

Open
XinyueZ opened this Issue Jun 29, 2018 · 27 comments

Comments

Projects
None yet
5 participants
@XinyueZ
Contributor

XinyueZ commented Jun 29, 2018

After converting to single-activity, there're two toolbars on detail screen.
The app needs a ux/ui decision what should be shown in detail page. @tiembo

@tiembo tiembo self-assigned this Jun 29, 2018

@tiembo

This comment has been minimized.

Collaborator

tiembo commented Jun 29, 2018

Good catch @XinyueZ - and indeed a good time to think about how the detail page should look. The Session Details page in the Google I/O 2018 Android app can serve as a good baseline of implementing a bottom app bar + FAB, along with title and details.

What are your thoughts?

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jun 29, 2018

The IO app can be a benchmark for detail screen. @tiembo that's great. I try some works to make similar effect.

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 1, 2018

@tiembo and @XinyueZ Sounds good!

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 2, 2018

@tiembo A way to fix duplicated appbar has been in PR(#78).

The the I/O-Like approach of detail screen might be worked with mutli-activities. The problem is if we do that, one of the appbar must be removed and keep one appbar(main), this should change struct of layout of detail screen, any suggestion? @sagar-viradiya

@andhie

This comment has been minimized.

Contributor

andhie commented Jul 2, 2018

the I//O 2018 app uses a separate activity .ui.sessiondetail.SessionDetailActivity to achieve it.
are there plans to revert the commit?
i think the proposed solution is by putting the AppBar in individual fragment. this helps to cater different AppBar styles.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 2, 2018

@andhie What's your opinion to #78 ?

@andhie

This comment has been minimized.

Contributor

andhie commented Jul 2, 2018

it feels cheating. and not so elegant solution.
it sends out a wrong idea that this is how you're suppose to do it when faced with multiple AppBar situation.
at this moment, i am looking for more patterns on how to use Navigation in cases like this. e.g. different style AppBar, no AppBar.

Hopefully @ianhanniballake could shed some light on such cases.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 2, 2018

@andhie yes, i c,
it's really a new pattern what people should face with Navi-Controller because the pro-suggestion of Google self is using single-activity with different fragments, however, there is no workaround at this moment to deal it with a method that was intended to be resolved with multi-activity. 🤕

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 2, 2018

@XinyueZ Even I have similar thoughts on your changes as @andhie mentioned above. If we are sticking with single activity then we should have appbar for each fragment. Having single appbar in main activity will cause problem if you have new fragment tomorrow and it has different requirement than main activity appbar.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 2, 2018

@sagar-viradiya Do you agree to have appbar or toolbar on each fragment? I am not against, actually, I have worked a potential PR which based on this direction. The reason why don't select for the potential PR is that I wouldn't change base struct of all these layouts and keep simple at min changes. @andhie How is your opinion?

@andhie

This comment has been minimized.

Contributor

andhie commented Jul 2, 2018

i remember the Google I/O Navigation talk mention to put AppBar/Toolbar inside each fragment. I would wait to listen Google's opinionated guide on this.

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 2, 2018

@XinyueZ I would prefer to have separate toolbar for each fragment. We need others opinion here. May be there could be better solution, but right now I could only think of this.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 3, 2018

@sagar-viradiya @andhie I have the alternative solution as PR #84, how about this? The approach is using toolbars for each fragment.

@tiembo

This comment has been minimized.

Collaborator

tiembo commented Jul 4, 2018

@andhie @sagar-viradiya @XinyueZ I'm going to talk to some folks internally regarding guidance for toolbars / single activity / bottom nav. Will follow up in about a week due to the US July 4th holiday.

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 4, 2018

@tiembo That would be great.

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 12, 2018

@tiembo Any update ?

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 12, 2018

@sagar-viradiya I prefer to #84

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Jul 12, 2018

@XinyueZ Will review and let you know

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 12, 2018

@sagar-viradiya thanks, #84 and #78 are two opposite solutions, one is multi appbar, one is single appbar, either of the two.

@tiembo

This comment has been minimized.

Collaborator

tiembo commented Jul 13, 2018

Hi everyone - we're working on a solution for toolbars with single Activity / multiple Fragments internally. Will update this issue once we have something to share.

@XinyueZ thanks for opening PRs #78 and #84! It's probably a good idea to hold off on making additional commits to those for now.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 13, 2018

@tiembo OK, looking forward to it.

@mcastagnolo

This comment has been minimized.

mcastagnolo commented Jul 20, 2018

It looks like the NavigationComponent in alpha03 added functionality to work with implementations where fragments own their toolbars.

https://developer.android.com/jetpack/docs/release-notes#july_12_2018
https://issuetracker.google.com/issues/109868820

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 20, 2018

I'll try it in a PR, @tiembo @sagar-viradiya it seems great.

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Jul 20, 2018

@tiembo

I am not sure whether it is really logical to put two different scenarios and businesses together.

official document, at last section mentions that
..... In cases where multiple Activities share the same layout, the navigation graphs can be combined ....

In our case they have actually different layout bases.

@sagar-viradiya

This comment has been minimized.

Contributor

sagar-viradiya commented Sep 5, 2018

@XinyueZ @tiembo Any plan to address this ?

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Sep 6, 2018

There're 2 PRs from my sides which could solve the problem, however, I prefer the #84 which uses the multi toolbar in different fragments for the case. Rather, still waiting for Google's official pattern. At moment there're conflicts, I'm just waiting for the merges of #170 and another associated PR of loading-ui @tiembo

@XinyueZ

This comment has been minimized.

Contributor

XinyueZ commented Nov 2, 2018

@tiembo Please check #238, the new version of nav-lib might be a chance to fix this issue, I guess only.
I will try a private branch with new lib to find way to fix the two-toolbar problem.

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