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(neon_dashboard): Remove scrolling inside dashboard widgets #1107

Merged
merged 1 commit into from
Dec 30, 2023

Conversation

provokateurin
Copy link
Member

@provokateurin provokateurin commented Nov 4, 2023

Fixes #1075

@provokateurin provokateurin force-pushed the fix/neon_dashboard/widget-scrolling branch from aeedfac to 1261278 Compare November 4, 2023 11:58
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

I don't know if I like this approach.
I haven't had a look at the performance but I don't think it will look good.

It looks like the web interface does only render 7 elements and at most one button (counting as an element).
Maybe we should also go this route.

packages/neon/neon_dashboard/lib/src/pages/main.dart Outdated Show resolved Hide resolved
packages/neon/neon_dashboard/lib/src/pages/main.dart Outdated Show resolved Hide resolved
@provokateurin
Copy link
Member Author

I understand your concern, but I found this to be the easiest way. Otherwise we have to calculate how many buttons or messages there are and then hide some items accordingly. It gets even harder because the messages and the buttons have different heights.

I'm not sure why you think the performance would be bad (except for calling the generator function twice). After building the widgets like you would normally do we just loop over them once more and read the size. Should be linear complexity so not really concerning?

@Leptopoda
Copy link
Member

https://api.flutter.dev/flutter/widgets/IntrinsicHeight-class.html

This class is relatively expensive, because it adds a speculative layout pass before the final layout phase. Avoid using it where possible. In the worst case, this widget can result in a layout that is O(N²) in the depth of the tree.

And this on top of the performance impact you already mentioned. I also dislike that this could potentially result in one list being extremely long forcing others to add more whitespace. This is not pleasing to look at.

@Leptopoda
Copy link
Member

But if you want I can test the performance impact we can move forward for now.

@provokateurin
Copy link
Member Author

And this on top of the performance impact you already mentioned

Ah I forgot about this :/

I also dislike that this could potentially result in one list being extremely long forcing others to add more whitespace. This is not pleasing to look at.

I think the chance is quite low. Usually there is 7 items, 1 button and 1 message at max. Technically there could be more than that but I don't think any app actually does that.

But if you want I can test the performance impact we can move forward for now.

If you don't mind please do.

@provokateurin provokateurin force-pushed the fix/neon_dashboard/widget-scrolling branch 2 times, most recently from b565496 to ea0f008 Compare November 16, 2023 20:36
@Leptopoda
Copy link
Member

Usually there is 7 items, 1 button and 1 message at max.

I currently have two buttons for notes ("Neue Notiz erstellen" and "Weitere Notizen").
I hope we can programatically limit the number of children.

If you don't mind please do.

Sure. I'll update you in a bit (maybe tomorrow).

@provokateurin provokateurin force-pushed the fix/neon_dashboard/widget-scrolling branch from ea0f008 to 325e2f3 Compare December 5, 2023 16:30
@provokateurin
Copy link
Member Author

@Leptopoda ping :)

@provokateurin provokateurin force-pushed the fix/neon_dashboard/widget-scrolling branch from 325e2f3 to b38de33 Compare December 15, 2023 14:36
@provokateurin provokateurin force-pushed the fix/neon_dashboard/widget-scrolling branch from b38de33 to 62e9925 Compare December 23, 2023 09:04
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

Let's just get it done.
Due to the dexing issues I could not verify the impact on a low end phone. There was no measurable impact on my x86 machine.

@provokateurin provokateurin merged commit 579ed35 into main Dec 30, 2023
8 checks passed
@provokateurin provokateurin deleted the fix/neon_dashboard/widget-scrolling branch December 30, 2023 09:37
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.

Force dashboard widgets to have a fixed height without scrolling
2 participants