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.

Sign up for GitHub

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

Jump to bottom

fix: added configurable padding #133

Merged
merged 5 commits into from
Nov 14, 2024
Merged

fix: added configurable padding #133

merged 5 commits into from
Nov 14, 2024

Conversation

vanlooverenkoen
Copy link
Contributor

Description

Add the option to use padding around a GoldenTestScenario

Type of Change

  • ✨ 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

btrautmann
btrautmann reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

Is this purely used for aesthetics?

@vanlooverenkoen
Copy link
Contributor Author

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
Copy link
Contributor

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 👍

btrautmann
btrautmann reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

Can we just add a test for this?

@vanlooverenkoen
Copy link
Contributor Author

Test added ✅

jeroen-meijer
jeroen-meijer reviewed Oct 3, 2024
View reviewed changes
Copy link
Collaborator

@jeroen-meijer jeroen-meijer left a comment

Choose a reason for hiding this comment

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

Couple comments.

I'll leave the rest to @btrautmann. Nice job!

lib/src/golden_test_theme.dart Outdated Show resolved Hide resolved
lib/src/golden_test_theme.dart Outdated Show resolved Hide resolved
test/src/golden_test_adapter_test.dart Outdated Show resolved Hide resolved
test/src/golden_test_adapter_test.dart Show resolved Hide resolved
@mx1up
Copy link

mx1up commented Oct 7, 2024

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.

@btrautmann
Copy link
Contributor

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
Copy link

mx1up commented Oct 7, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

@btrautmann fixed all comments

@mx1up
Copy link

mx1up commented Oct 9, 2024

@btrautmann fixed all comments

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

@vanlooverenkoen
Copy link
Contributor Author

Nice!! Do you like the documentation I added?

@mx1up
Copy link

mx1up commented Oct 9, 2024

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
Copy link
Contributor

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

@mx1up
Copy link

mx1up commented Nov 12, 2024

@vanlooverenkoen any progress on this? 🙏

@vanlooverenkoen
Merge branch 'main' into fix/padding
fa73493 
# Conflicts:
#	lib/src/golden_test_scenario.dart
#	lib/src/golden_test_theme.dart
@vanlooverenkoen
Copy link
Contributor Author

vanlooverenkoen commented Nov 14, 2024 •
edited
Loading

Trying to fix these broken tests right now

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

@vanlooverenkoen
Copy link
Contributor Author

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

btrautmann
btrautmann approved these changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@btrautmann btrautmann left a comment

Choose a reason for hiding this comment

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

domain lgtm 🚀
platform lgtm 💪

@btrautmann btrautmann merged commit ba5e4b3 into Betterment:main Nov 14, 2024
8 checks passed
@vanlooverenkoen vanlooverenkoen deleted the fix/padding branch November 15, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeroen-meijer jeroen-meijer jeroen-meijer left review comments

@mx1up mx1up mx1up left review comments

@btrautmann btrautmann btrautmann approved these changes

@samandmoore samandmoore Awaiting requested review from samandmoore samandmoore is a code owner

@Kirpal Kirpal Awaiting requested review from Kirpal Kirpal is a code owner

@jolexxa jolexxa Awaiting requested review from jolexxa jolexxa is a code owner

@marcossevilla marcossevilla Awaiting requested review from marcossevilla marcossevilla is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vanlooverenkoen @btrautmann @mx1up @jeroen-meijer