-
Notifications
You must be signed in to change notification settings - Fork 3
Improve Gutter #4
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
Conversation
Love this PR! I just visit the repo to fork it and implement this 👍 Thanks :-) |
What do you mean by "visit the repo to fork it and implement this"? Is there any changes needed, or do you want to merge this? |
I meant that I was start to working on implementing similar improvements when I stumbled upon your PR. 😁 u just saved me some time 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I had a handful of nits and one or two larger questions.
FYI I think I added a ruleset that enables the CI stuff you added, but not totally sure so let me know if that's not working.
@caseycrogers I'll get back to some more changes tomorrow. The CI still doesn't work so it would be great if you can fix that. |
I think your CI/CD won't work until after the PR is merged, no? If you want you could send me a separate PR with just the CI/CD stuff and I can merge it and see if that fixes it. |
@caseycrogers I've fixed the issues and made a separate PR for CI in case you want to merge that one in first. Let me know if you have any other feedback. |
@caseycrogers you need to enable CD in pub dev for deployment to work. Go to https://pub.dev/packages/flutter_gutter/admin Select: Enable publishing from GitHub Actions |
I left one final review comment on |
Sounds good! We use SlotLayout quite a bit in our apps, especially to scale things on web or windowed screens that can resize on the fly. That's probably why i would like to use it this way, but it is just a small wrapper and could be done in our app code as well. I thought it would be useful to others as well maybe. |
The build works now! After you've fixed the admin access things on pub you can merge this, make a v2.0.0 tag on the repo and it will do the work for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'll ship this later today, thanks a lot of the contribution I really appreciate it!!
materialSpacing * Gutter.scaleFactorMediumDefault, | ||
GutterType.expand => double.infinity, | ||
} | ||
..floor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the floor
here? Shouldn't Flutter handle any non-pixel boundaries when it converts to physical pixels?
Also, if we DO want to keep the floor in, shouldn't it apply after multiplying by scale factor?
Either way I'll just merge and ship this now and if we want to pull the floor back out I'll handle that in a patch version because I don't want to leave you stuck in async review h*ll.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making some more changes right now. Just found some stuff. I'll also add some tests
Add optional size and scaleFactor parameters
Introduce constructors on Gutter for types like small, large, etc
Add Gutter.expand
Add extension to add Gutter on Lists
Add AdaptiveGutter that switches Gutter size based on the current breakpoint
Migrate to flutter_adaptive_scaffold to remove deprecated package