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

Padding implementation #7

Closed
wants to merge 13 commits into from
Closed

Conversation

martijn00
Copy link
Contributor

@martijn00 martijn00 commented Aug 2, 2024

Connection with issue(s)

Connected to #6

Solution description

Once PR 6 is merged in i'll rebase this so the rest is gone.

I've now set the values the a double? type, but the disadvantage of this is that you cannot let every direction calculate its own value. Say i want a Gutter.medium top and bottom, which would use the default material value, but i want a left and right gutter.small(scaleFactor = 4). That is why i put the Gutter widget in.

Using Gutter:

const GutterPadding.all(
    value: Gutter.medium(),
    child: ColoredBox(color: Colors.blue, child: Text('Child')),
),

const GutterPadding.only(
    left: Gutter.medium(),
    right: Gutter.small(),
    top: Gutter(size: 20),
    bottom: Gutter.medium(scaleFactor: 3),
    child: ColoredBox(color: Colors.blue, child: Text('Child')),
),

Using double? as parameter:

const GutterPadding.all(
    value: context.gutterSize(
      type: GutterType.medium,
    ),
    child: ColoredBox(color: Colors.blue, child: Text('Child')),
),

const GutterPadding.only(
    left: null,
    right: context.gutterSize(
      type: GutterType.small,
    ),
    top: context.gutterSize(
      type: GutterType.medium,
      size: 20
    ),
    bottom: context.gutterSize(
      type: GutterType.medium,
      scaleFactor: 3
    ),
    child: ColoredBox(color: Colors.blue, child: Text('Child')),
),

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

@martijn00 martijn00 changed the title Padding2 Padding implementation Aug 2, 2024
@martijn00 martijn00 mentioned this pull request Aug 2, 2024
4 tasks
@caseycrogers
Copy link
Owner

Aww I see there were some other changes here too.

In the old version I could do: context.gutter, context.gutterSmall etc and directly get the double value of the gutter. It feels to me like context.gutterSize() and GutterType are really more of implementation details and are pretty clunky to use as a part of the external API.

Can we go back to directly exposing context.gutterSmall etc and remove gutterSize or make it a private implementation detail? It'll make the source code a bit more tedious but it'll make the external API dramatically more intuitive and easy to use.

@caseycrogers
Copy link
Owner

Also, in T-minus 3 hours I'll be unreachable for a week so maybe it's best we table this and I get back to you after?

@martijn00
Copy link
Contributor Author

@caseycrogers sure, no worries. Let me know when you are back and we'll discuss it further. I'll close this PR and continue any work in #6

@martijn00 martijn00 closed this Aug 3, 2024
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