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

update the sunflower sample to perform better at various sizes #2836

Merged
merged 6 commits into from
Feb 15, 2024

Conversation

devoncarew
Copy link
Member

  • a minor update to the sunflower sample - have it perform better at various sizes (constrained height, ...)

Previously, this sample would overflow if the containing area was not tall enough.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

Looks good to me! This is nice and ends up looking better in both a constrained and wider environment.

One optional consideration to consider:

Could you add some spacing after(below) the slider? It felt a bit cramped interacting with it so close to the bottom, and it might feel even more so on mobile.

@parlough
Copy link
Member

Also note that you'll need to update this PR and regenerate since cff1de0 landed.

@devoncarew
Copy link
Member Author

Could you add some spacing after(below) the slider? It felt a bit cramped interacting with it so close to the bottom, and it might feel even more so on mobile.

👍

Copy link
Member Author

@devoncarew devoncarew left a comment

Choose a reason for hiding this comment

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

ptal again - I updated the sample w/ improved padding and to better match modern best practices

@devoncarew
Copy link
Member Author

Screenshot 2024-02-15 at 9 14 07 AM

Copy link
Member

@parlough parlough left a comment

Choose a reason for hiding this comment

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

That looks good to me! Thanks for the adjustments.

Maybe we should just drop the drawer? I'm not sure what it's adding in relation to the current focus of the sample. We can show that off in some other sample where it fits better contextually.

@johnpryan
Copy link
Contributor

We don't need the drawer, we added it a long time ago to prove that this was a Flutter app and not an HTML app

@devoncarew
Copy link
Member Author

Gotcha - will take out the drawer.

@devoncarew
Copy link
Member Author

done! The sample's just over 100 lines of code now; I think if we wanted to we could get it down to ~100.

@devoncarew devoncarew merged commit 2378afc into main Feb 15, 2024
@devoncarew devoncarew deleted the update_sunflower branch February 15, 2024 18:03
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.

3 participants