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

feat: ✨ Tooltip action widget #358

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: ✨ Tooltip action widget #358

wants to merge 1 commit into from

Conversation

faiyaz-shaikh
Copy link
Contributor

@faiyaz-shaikh faiyaz-shaikh commented Mar 16, 2023 •
edited

Description

✨ Added Tooltip action widget

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

@faiyaz-shaikh faiyaz-shaikh changed the title ✨ Tooltip action widget feat: ✨ Tooltip action widget Mar 16, 2023
DhvanitVaghani
DhvanitVaghani reviewed Mar 21, 2023
View reviewed changes
lib/src/tooltip_action.dart Outdated
alignment: WrapAlignment.spaceBetween,
crossAxisAlignment: WrapCrossAlignment.center,
children: [
getActionWidget(actionOne),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pass List<ActionWidgetConfig> here? I guess the user gets more feasibility to add more than two actions.

@faiyaz-shaikh faiyaz-shaikh force-pushed the feature/tooltip_action_widget branch 2 times, most recently from eee3129 to 15896f6 Compare March 24, 2023 09:15
@faiyaz-shaikh faiyaz-shaikh force-pushed the feature/tooltip_action_widget branch 5 times, most recently from 76bab00 to a7c83ff Compare March 30, 2023 12:12
@faiyaz-shaikh faiyaz-shaikh force-pushed the feature/tooltip_action_widget branch 2 times, most recently from cf90e13 to 6fa0643 Compare March 31, 2023 12:00
ujas-m-simformsolutions
ujas-m-simformsolutions requested changes Apr 6, 2023
View reviewed changes
lib/src/tooltip_action.dart Outdated
}

Widget getActionWidget(ActionWidgetConfig? actionWidgetConfig) {
if (actionWidgetConfig == null) return const SizedBox();
Copy link
Contributor

Choose a reason for hiding this comment

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

use SizedBox.shrink() and can you please check if we turn on flutter inspector it doesn't make whole screen grey when actionWidgetConfig is null.

ujas-m-simformsolutions
ujas-m-simformsolutions requested changes Apr 12, 2023
View reviewed changes
lib/src/tooltip_action.dart Outdated
child: Wrap(
alignment: alignment,
crossAxisAlignment: crossAxisAlignment,
children: actions != null
Copy link
Contributor

Choose a reason for hiding this comment

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

Also check if actions is empty or not

lib/src/tooltip_action.dart Outdated

const ToolTipAction.customAction({
Key? key,
this.actions,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for custom actions this should be a required field

lib/src/tooltip_action.dart Outdated
this.padding,
this.margin,
this.textStyle,
required this.onActionTap,
Copy link
Contributor

Choose a reason for hiding this comment

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

Required fields should be at top

lib/src/tooltip_widget.dart Outdated
}

// ignore: unused_element
Copy link
Contributor

Choose a reason for hiding this comment

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

If not using this function why not remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have started using this function so no need to remove it.

lib/src/tooltip_widget.dart Outdated
),
],
),
SizedBox(child: widget.toolTipAction),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we add this without a SizedBox

lib/src/showcase.dart Outdated
@@ -232,6 +234,9 @@ class Showcase extends StatefulWidget {
/// Provides padding around the description. Default padding is zero.
final EdgeInsets? descriptionPadding;

/// Provides tooltip action
Copy link
Contributor

Choose a reason for hiding this comment

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

Please describe more on what this field does.

lib/src/tooltip_action.dart Outdated
),
),
getActionWidget(
actionTwo ??
Copy link
Contributor

Choose a reason for hiding this comment

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

What if user doesn't want to provide 2nd action. I think Instead of making user to pass a sizedbox, we should only show this if they have provided the config.

@aditya-css aditya-css added the enhancement New feature or request label Mar 5, 2024
@Sahil-Simform Sahil-Simform force-pushed the feature/tooltip_action_widget branch 3 times, most recently from 61a3f12 to 84a5883 Compare March 18, 2024 08:49
@faiyaz-shaikh @Sahil-Simform
feature: ✨ Tooltip action widget
f4a88fb 
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sahil-Simform Sahil-Simform Sahil-Simform left review comments

@ujas-m-simformsolutions ujas-m-simformsolutions ujas-m-simformsolutions requested changes

@DhvanitVaghani DhvanitVaghani DhvanitVaghani approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@faiyaz-shaikh @DhvanitVaghani @ujas-m-simformsolutions @Sahil-Simform @aditya-css