Skip to content
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

Fix black flick #23

Closed
wants to merge 4 commits into from
Closed

Fix black flick #23

wants to merge 4 commits into from

Conversation

charbgr
Copy link

@charbgr charbgr commented Jul 24, 2015

Suppose we have two fragments. Fragment A and Fragment B. When there is a transition between B --> A, bottomsheet flicks with a black screen causing transition seems awful.

dimView.setBackgroundColor must be set to Color.TRANSPARENT.

@ZacSweers
Copy link
Contributor

Could you link a gif or screenrecord of the issue?

@charbgr
Copy link
Author

charbgr commented Jul 24, 2015

Sure! Check this out! https://www.youtube.com/watch?v=2BzbmDg6Nxo at 0:13.

Sometimes doesn't flick.. :/

@ZacSweers
Copy link
Contributor

That video's with or without the proposed fix?

@charbgr
Copy link
Author

charbgr commented Jul 25, 2015

without

2015-07-25 7:49 GMT+03:00 Henri (Zac) Sweers notifications@github.com:

That video's with or without the proposed fix?


Reply to this email directly or view it on GitHub
#23 (comment).

@SamThompson
Copy link
Contributor

I agree this is a bug, but we can't set dimView to be transparent because we would lose the dimming effect. The black screen no longer showed up probably because by making dimView transparent, it was effectively being hidden from view.

@charbgr
Copy link
Author

charbgr commented Jul 27, 2015

So, what if we change this dynamically?

@SamThompson
Copy link
Contributor

Could you supply a code example of the bug? We're having trouble reproducing it.

@charbgr
Copy link
Author

charbgr commented Jul 31, 2015

git clone git@bitbucket.org:charbgr/bottomsheet-black_flick.git

@ZacSweers
Copy link
Contributor

Get a permission denied error when trying to clone :/

On Fri, Jul 31, 2015 at 3:22 AM Basilis Charalampakis <
notifications@github.com> wrote:

git clone git@bitbucket.org:charbgr/bottomsheet-black_flick.git


Reply to this email directly or view it on GitHub
#23 (comment).

@charbgr
Copy link
Author

charbgr commented Aug 2, 2015

Its not private.. try htts : git clone
https://charbgr@bitbucket.org/charbgr/bottomsheet-black_flick.git

2015-08-02 1:44 GMT+03:00 Henri (Zac) Sweers notifications@github.com:

Get a permission denied error when trying to clone :/

On Fri, Jul 31, 2015 at 3:22 AM Basilis Charalampakis <
notifications@github.com> wrote:

git clone git@bitbucket.org:charbgr/bottomsheet-black_flick.git


Reply to this email directly or view it on GitHub
<#23 (comment)
.


Reply to this email directly or view it on GitHub
#23 (comment).

@ZacSweers
Copy link
Contributor

That worked. Looking at it now though, I see that you're using a bottomsheet in the rootview for your fragment but not you're activity. To be honest, I'm not sure that's a case we want to support. Bottomsheet is supposed to be relative to the entire content view, and it would be weird (in my opinion) to show it in smaller containers on the screen. The material spec doesn't show any examples of this either. We have some upcoming changes that also assume that it's relative to the entire screen, so for the forseeable future I think it's best to stick with it on a per-activity basis.

Testing your sample code, I was able to remove the issue after moving it to the main activity layout. Here's a patch file of the changes if you want to apply and test yourself.

If you absolutely need to be able to do this on a per-fragment basis, I'd rather the pull request tune parts of BottomSheetLayout that would allow you to extend and override this behavior locally. What do you think?

@ZacSweers
Copy link
Contributor

Going to close this for now

@ZacSweers ZacSweers closed this Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants