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

Improvements and Tests #6

Merged
merged 26 commits into from
Aug 12, 2024
Merged

Improvements and Tests #6

merged 26 commits into from
Aug 12, 2024

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented Aug 2, 2024

Connection with issue(s)

Connected to #4

Solution description

Screenshots or Videos

To Do

  • Check the original issue to confirm it is fully satisfied
  • Add solution description to help guide reviewers
  • Add unit test to verify new or fixed behavior
  • If apply, add documentation to code properties and package readme

@caseycrogers
Copy link
Owner

Will wait on release! Just lmk when this is ready for merge and we're ready for launch

Though heads up I'm leaving for a ~6 day camping trip in 4 hours so it'll be awhile before I get back to this

@martijn00
Copy link
Contributor Author

@caseycrogers Done! I'll open a new PR if i find anything but this should be good to merge and release.

@martijn00
Copy link
Contributor Author

@caseycrogers Done

@martijn00 martijn00 mentioned this pull request Aug 2, 2024
4 tasks
lib/src/gutter_padding.dart Outdated Show resolved Hide resolved
@martijn00
Copy link
Contributor Author

@caseycrogers if you have some time when you are back can we look at getting this merged?

I'm also making some PRs to flutter adaptive scaffold itself to make some checks in here easier.

Copy link
Owner

@caseycrogers caseycrogers left a comment

Choose a reason for hiding this comment

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

I feel bad dragging this out so much but I don't think this PR is in a state I feel good about yet. My main requested changes are removing new features. Part of the problem is that the PR includes a refactor of the implementation details, a refactor of the external API, and several unrelated new features so it becomes hard to get out the door so I'm proposing more or less removing all the new features.

Once you remove the things I mentioned I'll merge this and then do some quick housecleaning (EDIT: actually some fairly heavy changes, namely rolling back to the GutterTiny/GutterSmall/etc system) on top of it and ship it out.

If you're burnt out on working on the PR given how much back and forth this has been let me know and I'll do all the changes I requested myself and merge and ship-I really appreciate your help and feel bad for taking up so much of your time.

lib/src/gutter_padding.dart Outdated Show resolved Hide resolved
lib/src/gutter_extensions.dart Show resolved Hide resolved
lib/src/gutter_extensions.dart Outdated Show resolved Hide resolved
lib/src/gutter.dart Show resolved Hide resolved
lib/src/gutter.dart Outdated Show resolved Hide resolved
lib/src/gutter_type.dart Outdated Show resolved Hide resolved
lib/src/gutter.dart Show resolved Hide resolved
@martijn00
Copy link
Contributor Author

I've tried to accommodate your change requests. I really don't think rolling back to the GutterTiny etc and Gap is a good idea. Rather move forward and go to a RenderBox.

Copy link
Owner

@caseycrogers caseycrogers left a comment

Choose a reason for hiding this comment

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

There's a good bit of stuff I want to go through in this but I'm just going to merge it and do it on my end. I'll do my changes in a PR so you can see what I changed and what will eventually ship.

WRT to rolling back to Gap etc vs going forward to RenderBox, the former is the external APi and the latter is a perf centric implementation detail. IMO the two are totally independent-we should definitely switch the implementation to a RenderBox, it's just a matter of getting to it. I prefer the Gap/multiple widget API, but would love to hear why you prefer the single widget multiple constructors approach so we can consider migrating the external API to that

@caseycrogers caseycrogers merged commit 6777f1d into caseycrogers:main Aug 12, 2024
3 checks passed
@martijn00
Copy link
Contributor Author

martijn00 commented Aug 12, 2024

I've made flutter/packages#7300 here you can see the usage of multi constructors as well: https://github.com/flutter/packages/pull/7300/files#diff-8399b95990f0c1ed9192ac5d83957d04277e6d3b8b5add66bed1bb43b608f9e9R119

After flutter/packages#7347 is merged I'll make a PR to update gutter to use it as well.

I'll also open some PR's to the adaptive package to add official sizes for the gap, hopefully that lands sometime soon.

It's ok if you prefer multiple widget, but i've made both ways possible right now, so why not leave both ways in?

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.

2 participants