fix: added configurable padding

[vanlooverenkoen]

Description

Add the option to use padding around a GoldenTestScenario

Type of Change

• [X] ✨ New feature (non-breaking change which adds functionality)
• [ ] 🛠️ Bug fix (non-breaking change which fixes an issue)
• [ ] ❌ Breaking change (fix or feature that would cause existing functionality to change)
• [ ] 🧹 Code refactor
• [ ] ✅ Build configuration change
• [ ] 📝 Documentation
• [ ] 🗑️ Chore

[vanlooverenkoen]

Because we have a lot of GoldenTestScenarios with the removal of the padding widget it is a lot harder to do visual pr review. The cognitive load is much higher when there is no padding. (I get that this is not required for everyone, that is why I made it optionally configurable)

[btrautmann]

Because we have a lot of GoldenTestScenarios with the removal of the padding widget it is a lot harder to do visual pr review. The cognitive load is much higher when there is no padding. (I get that this is not required for everyone, that is why I made it optionally configurable)

Understood. Yeah I think GoldenTestTheme created a nice place to put these optional parameters. I'm cool with this 👍

[vanlooverenkoen]

Test added ✅

[mx1up]

Is this purely used for aesthetics?

it seems in 0.10.0, a breaking change was introduced that removed the padding ( 5139917e4064811c2dadc153ffcf40f07f102459 ). But since it is not documented, maybe it was an accidental change? If so, it might be better to introduce a default padding of 8dp as it used to be before.

[btrautmann]

Is this purely used for aesthetics?

it seems in 0.10.0, a breaking change was introduced that removed the padding ( 5139917 ). But since it is not documented, maybe it was an accidental change? If so, it might be better to introduce a default padding of 8dp as it used to be before.

The change was intentional, I'd rather that padding be configurable than to add it back.

[mx1up]

it seems in 0.10.0, a breaking change was introduced that removed the padding ( 5139917 ). But since it is not documented, maybe it was an accidental change? If so, it might be better to introduce a default padding of 8dp as it used to be before.

The change was intentional, I'd rather that padding be configurable than to add it back.

ok, fine. just a suggestion: it might be helpful to document this change somewhere for users migrating to 0.10+

[btrautmann]

ok, fine. just a suggestion: it might be helpful to document this change somewhere for users migrating to 0.10+

Yeah, apologies for not being more transparent in the changelog about the change. Once this lands (CC @vanlooverenkoen) I'll make sure to make note of it in the changelog indicating the correct path for migrating.

[vanlooverenkoen]

@btrautmann fixed all comments

[mx1up]

@btrautmann fixed all comments

thanks @vanlooverenkoen , i can confirm, using this branch, all our golden tests are green again 🥳

[vanlooverenkoen]

Nice!! Do you like the documentation I added?

[mx1up]

Nice!! Do you like the documentation I added?

yes, clear to me 👌

I don't know if this project follows Effective dart doc recommendations though, specifically:

DO start doc comments with a single-sentence summary

[btrautmann]

@vanlooverenkoen thanks for updates; unfortunately it looks like we have a couple failing tests.

[mx1up]

@vanlooverenkoen any progress on this? 🙏

[vanlooverenkoen]

Trying to fix these broken tests right now

Sorry @mx1up and @btrautmann this got out of sight for a moment

[vanlooverenkoen]

@mx1up & @btrautmann should be fixed (at least locally)