Conversation
This callback add in params a RelativeRect position object like onTapActionAtPositionCallback but with the top left of the button instead of the tap position.
By leveraging the new TmailButtonWidget, we can remove totally the need of a GlobalKey. Code is cleaner and no memory leak possible.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| return TMailContainerWidget( | ||
| onTapActionCallback: onTapActionCallback, | ||
| onTapActionAtPositionCallback: onTapActionAtPositionCallback, | ||
| onTapActionAtOriginCallback: onTapActionAtOriginCallback, |
There was a problem hiding this comment.
I see this as a common pattern in this changeset that the calling context forward tons of of argument thought class fields downsteam.
While i understand this is an established pattern that we might not want to challenge here ( :-( ) I would like us to think how we Improve the overall structure of the code over here.
Ideas (uncontextualized) out of the box:
- Grouping arguments together within case class
EG
case class WidgetStyle(Width with, Height height, MaxWidth maxWidth, MaxHeight maxHeight, BorderRadius borderRadius)
case class WidgetInteractions(onTapActionCallback: onTapActionCallback, onTapActionAtPositionCallback, onTapActionAtOriginCallback, onTapActionAtOriginCallback);
And return TMailContainerWidget(widgetInteractions, widgetStyle, child: childWidget);
Bonus points: style convertions would be in WidgetStyle, interactions transformation in the case class as well
Extra bonus point - breaking evil combo: reuse.In over class
-
Other idea 2 staged builders: we only build a minimal object to which the caller would add it's state so that this class do not need to be aware.
-
Very likely zillion of other good ideas.
Cc @hoangdat
|
We don't need this anymore, please close it. @zatteo |
3 participants
No description provided.