-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: implement custom promotionbox (#294) #295
Conversation
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.
Thanks for contributing! Looks good and makes customization possible!
Three things before merging and release:
- The non const default constructor might be a breaking change (would like to avoid that) (alternative would be to separate the content creation into a const private Widget?)
- Missing documentation (at least Changelog)
- Missing Semantics (can add this separately though)
], | ||
if (widget.leading != null) | ||
Padding( | ||
padding: const EdgeInsets.fromLTRB(0, 0, 8.0, 0), |
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.
EdgeInsets.only(right: 8.0)
Also not so sure whether we should just add padding by default or leave that to the user ... would prefer that
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 would leave the padding, otherwise we could just provide a "content" widget and not even bother with leading and trailing
@@ -7,26 +7,125 @@ import 'promotion_badge.dart'; | |||
part 'promotion_box.assets.dart'; | |||
|
|||
class SBBPromotionBox extends StatefulWidget { | |||
const SBBPromotionBox({ |
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.
hm, thinking whether we would need to treat the non const constructor as a breaking change...
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 think so
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.
Will treat it as a Breaking change and adapt the Changelog in separate PR accordingly.
Thanks @Grodien for the contribution!
No description provided.