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

Previewing a component that is scrollable #138

Closed
Dambakk opened this issue Mar 15, 2021 · 14 comments · Fixed by #163
Closed

Previewing a component that is scrollable #138

Dambakk opened this issue Mar 15, 2021 · 14 comments · Fixed by #163
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@Dambakk
Copy link

Dambakk commented Mar 15, 2021

Hi!

I have a component that uses the verticalScroll modifier. Previewing it in AS works fine, but when opening the group which contains this composable in the Showkase activity, I get an exception saying:

java.lang.IllegalStateException: Nesting scrollable in the same direction layouts like ScrollableContainer and LazyColumn is not allowed. If you want to add a header before the list of items please take a look on LazyColumn component which has a DSL api which allows to first add a header via item() function and then the list of items via items().

How should I go about resolving this, or isn't this functionality taken into account yet?

@vinaygaba
Copy link
Collaborator

@Dambakk Thanks for flagging this! I haven't tested with this so I would say that its currently unsupported but definitely feel that support for this should exist. I'll take a stab at this later today and report my findings!

@vinaygaba vinaygaba added bug Something isn't working enhancement New feature or request labels Mar 16, 2021
@vinaygaba vinaygaba self-assigned this Mar 16, 2021
@vinaygaba
Copy link
Collaborator

@Dambakk Any chance you could share your component that used the verticalScroll modifier here so that its easier to repro and fix!

@Dambakk
Copy link
Author

Dambakk commented Mar 17, 2021

Of course! So this is taken from the official verticalScroll modifier example in the docs:

@Composable
fun VerticalScrollSample() {
    Column(
        modifier = Modifier
            .background(Color.LightGray)
            .fillMaxSize()
            .verticalScroll(rememberScrollState())
    ) {
        repeat(100) {
            Text("Item $it", modifier = Modifier.padding(2.dp))
        }
    }
}

@Preview
@Composable
fun VerticalScrollPreview() {
    VerticalScrollSample()
}

except that the size is fill... instead of fixed. It works fine (that is the browser activity does not crash) when the composable has a fixed height, but crashes with the exception pasted above when the size (or height when using vertical scroll I guess) is fillMax...

@vinaygaba, are you able to reproduce the issue with this snippet?

@vinaygaba
Copy link
Collaborator

vinaygaba commented Mar 17, 2021

I was able to reproduce it with the snippet! I think the stacktrace message isn't totally accurate as you are able to scroll in the same direction correctly when you specify a fixed height to the composable(as you rightly pointed out). Think I need to do a bit more digging to find a fix for it. @Dambakk

@Dambakk
Copy link
Author

Dambakk commented Mar 18, 2021

Cool! Let me know if I can help with anything! 🙋

@Dambakk
Copy link
Author

Dambakk commented Apr 28, 2021

Hey! How's it going? Any progress yet?

@vinaygaba
Copy link
Collaborator

I'm going to give it a shot with the latest version of Showkase that's targeting Compose beta05. Will let you know what I find!

@L7ColWinters
Copy link

It still crashes :(

@paulnunezm
Copy link

Yeah, I can see the issue too :/

@vinaygaba
Copy link
Collaborator

vinaygaba commented Aug 3, 2021

I did some more research about why this error is even thrown. For anyone interested, here's the thread that explains why Google decided to add this check - https://kotlinlang.slack.com/archives/CJLTWPH7S/p1616434089403900?thread_ts=1616403538.380900&cid=CJLTWPH7S

In order to get around this issue, what you need to ensure is that when your component is in preview mode, it isn't trying to use fillMaxHeight. One way to do this would be to make sure that your component can take in a modifier and use a default modifier for the general case. However, for the preview, you pass in the height modifier and that should fix the issue. Here's what your code would look like if I modified the example you shared

@Composable
fun VerticalScrollSample(
    modifier: Modifier = Modifier
) {
    Column(
        modifier = modifier // In the default case, it will continue to use the max size
            .background(Color.LightGray)
            .fillMaxSize()
            .verticalScroll(rememberScrollState())
    ) {
        repeat(100) {
            Text("Item $it", modifier = Modifier.padding(2.dp))
        }
    }
}

@Preview(name = "Scrollable", group = "Debug Scrollable")
@Composable
fun VerticalScrollPreview() {
    VerticalScrollSample(
        Modifier.height(500.dp) // or some other reasonable value
    )
}

Let me know if this fixes the issue for you @Dambakk @L7ColWinters @paulnunezm

@Dambakk
Copy link
Author

Dambakk commented Aug 4, 2021

I'm on vacation for another week, but my initial thoughts is that what you describe will probably work, but that it feels like a workaround rather than a solution. It would be interesting to know more about how the AS preview handles this situation. Anyone at Google we can ask?

@vinaygaba
Copy link
Collaborator

@Dambakk Preview most likely doesn't encounter this issue because they aren't trying to render a scrollable composable inside a scrollable composable. The reason I encounter it is because the Showkase browser app is built using Compose itself and it renders the composable functions inside a LazyColumn. One way to potentially fix it would be to write the Showkase browser app using a RecyclerView but I'd be doing Showkase a disservice if I did that 😅

@Dambakk
Copy link
Author

Dambakk commented Aug 4, 2021

Right... Hmm.. I havent looked at the showkase source yet, but would it be possible to set like a max height/width for the rendered composable to e.g. the height of the screen? So basically, move the solution you suggested into the library but using a wrapping box rather than providing a modifier. Not sure, I guess it will still have nested scroll, Im just trying to think of solutions. Maybe the modifier is the way to go 🤷‍♂️

@vinaygaba
Copy link
Collaborator

Just pushed a PR that fixes this problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants