-
Notifications
You must be signed in to change notification settings - Fork 455
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: ❇️ Provide tooltip action buttons #466
feat: ❇️ Provide tooltip action buttons #466
Conversation
3507ecb
to
2408212
Compare
- Added `toolTipAction` parameter in `Showcase` which is used to get action widget configuration and defaults to `null` - Added `DefaultToolTipActionWidget` class for default tooltip action widgets
- Added `ToolTipActionButton` class for tooltip action buttons
34cb40f
to
685adf3
Compare
@@ -1,500 +0,0 @@ | |||
// !$*UTF8*$! |
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.
this file should not be changed.
example/lib/main.dart
Outdated
@@ -221,7 +236,34 @@ class _MailPageState extends State<MailPage> { | |||
"Tap to see profile which contains user's name, profile picture, mobile number and country", | |||
tooltipBackgroundColor: Theme.of(context).primaryColor, | |||
textColor: Colors.white, | |||
onTargetClick: () { |
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.
remove unrelated changes from this PR.
example/lib/main.dart
Outdated
backgroundColor: Colors.pink.shade50, | ||
textStyle: const TextStyle( | ||
color: Colors.pink, | ||
)), |
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.
format the code.
lib/src/showcase.dart
Outdated
@@ -26,11 +26,10 @@ import 'dart:ui'; | |||
import 'package:flutter/foundation.dart'; | |||
import 'package:flutter/material.dart'; | |||
|
|||
import 'enum.dart'; | |||
import '../showcaseview.dart'; |
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.
any reason for this import?
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.
No particular reason for this import; it just reduces the number of other imports needed.
Removing this and adding individual file imports.
lib/src/showcase.dart
Outdated
@@ -299,12 +309,14 @@ class Showcase extends StatefulWidget { | |||
this.tooltipPosition, | |||
this.titlePadding, | |||
this.descriptionPadding, | |||
this.tooltipActions, |
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.
put action and config together in constructor
lib/src/showcase.dart
Outdated
: widget.tooltipActions ?? []; | ||
|
||
final actionWidgets = <Widget>[]; | ||
for (var action = 0; action < actionData.length; action++) { |
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.
should use for in loop
lib/src/showcase.dart
Outdated
), | ||
], | ||
], | ||
); | ||
} | ||
|
||
List<Widget> _getTooltipActions() { | ||
final showCaseState = ShowCaseWidget.of(context); |
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.
use context check before this
lib/src/showcase.dart
Outdated
), | ||
child: TooltipActionButtonWidget( | ||
config: actionData[action], | ||
showCaseState: ShowCaseWidget.of(context), |
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.
Any reason to pass this state? We can access it directly from the widget.
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.
f2180b7
to
8b2c1fe
Compare
lib/src/tooltip_widget.dart
Outdated
var maxTextWidth = | ||
max(toolTipActionSize?.width ?? 0, max(titleLength, descriptionLength)); |
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.
At line 242, you have considered textPadding for condition but not here. Why is that?
lib/src/tooltip_widget.dart
Outdated
if ((toolTipActionSize?.width ?? 0) >= | ||
(max(titleLength, descriptionLength) + textPadding)) { | ||
tooltipWidth = toolTipActionSize?.width ?? 0; | ||
} else { | ||
tooltipWidth = maxTextWidth + textPadding; |
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.
Here can we use maxTextWidth
in some to simplify this?
lib/src/tooltip_widget.dart
Outdated
tempPos = Offset(position!.dx, position!.dy + size!.height); | ||
setState(() => position = tempPos); |
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.
Add mounted check and use ??
lib/src/widget/action_widget.dart
Outdated
padding: EdgeInsets.only( | ||
top: isArrowUp ? tooltipActionConfig.gapBetweenContentAndAction : 0.0, | ||
bottom: | ||
!isArrowUp ? tooltipActionConfig.gapBetweenContentAndAction : 0.0, | ||
).add(outSidePadding), |
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.
Do add operation in build.
lib/src/tooltip_widget.dart
Outdated
(widget.tooltipPadding ?? EdgeInsets.zero).horizontal + | ||
max((widget.titlePadding ?? EdgeInsets.zero).horizontal, | ||
(widget.descriptionPadding ?? EdgeInsets.zero).horizontal); |
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.
at this point just create a variable named zeroPadding.
b425f85
to
a2fd3db
Compare
a2fd3db
to
17af25d
Compare
52b6ca7
to
ac00df9
Compare
706a9f3
to
bcf824a
Compare
|
||
return config.button ?? | ||
GestureDetector( | ||
onTap: handleOnTap, |
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.
Can we check if the onTap working with the button containing a transparent background. Here there chance the tap event might be received by the background barrier tap.
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.
Yes it will work on transparent part for that user has to provide the padding as needed.
Updated Example application for this.
} | ||
|
||
final availableSpaceForText = | ||
(widget.position?.screenWidth ?? MediaQuery.of(context).size.width) - |
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.
Please add a documentation comment here explaining the logic of getting available space for text
bcf824a
to
da9b99b
Compare
da9b99b
to
2dad8a5
Compare
Description
This PR allows us to provide the list of tooltip action button with configurations.
Checklist
fix:
,feat:
,docs:
etc).docs
and added dartdoc comments with///
.examples
ordocs
.Breaking Change?
Video of the new feature
379697654-ed82cee2-f283-49e8-bfe5-a79f66e151eb.webm