-
-
Notifications
You must be signed in to change notification settings - Fork 625
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: shape and side for SlidableAction and CustomSlidableAction #255
base: master
Are you sure you want to change the base?
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 this nice evolution! I have just small requests to let the default values to be in one place only.
lib/src/actions.dart
Outdated
this.shape = const RoundedRectangleBorder(), | ||
this.side = BorderSide.none, |
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.
It would be better if we let them null by default so that we can easily extend this class and keep the default values without redefining them.
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.
thank you, fixed
lib/src/actions.dart
Outdated
/// [OutlinedBorder] of action, if is null, then [RoundedRectangleBorder] | ||
/// used by default | ||
/// {@endtemplate} | ||
final OutlinedBorder shape; |
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.
We should make OutlinedBorder
nullable then.
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.
fixed
lib/src/actions.dart
Outdated
/// {@template slidable.actions.side} | ||
/// [BorderSide] of action, if is null, then [BorderSide.none] used by default | ||
/// {@endtemplate} | ||
final BorderSide side; |
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.
We should make BorderSide
nullable too.
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.
fixed
lib/src/actions.dart
Outdated
@@ -123,6 +135,8 @@ class SlidableAction extends StatelessWidget { | |||
this.icon, | |||
this.spacing = 4, | |||
this.label, | |||
this.shape = const RoundedRectangleBorder(), |
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.
By following the previous comment, we dont' need to redefine them here.
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.
fixed
Co-authored-by: Romain Rastel <[email protected]>
@letsar thank you for your review! |
Hi, thank you very much for your awesome project!
This PR I would like to propose adding two small changes:
The shape and side for SlidableAction and CustomSlidableAction make SlidableAction more modifiable.
Thank you!