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.

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: ✨ Tooltip action widget #358

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

faiyaz-shaikh
Copy link
Contributor

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

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
alignment: WrapAlignment.spaceBetween,
crossAxisAlignment: WrapCrossAlignment.center,
children: [
getActionWidget(actionOne),
Copy link
Contributor

@DhvanitVaghani DhvanitVaghani Mar 21, 2023

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
}

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.

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


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

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

}

// 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.

),
],
),
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

@@ -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.

),
),
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 4 times, most recently from 84a5883 to f4a88fb Compare March 18, 2024 08:54
faiyaz-shaikh and others added 2 commits July 3, 2024 15:35
- 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
@rashi-simform rashi-simform force-pushed the feature/tooltip_action_widget branch from f4a88fb to 001c0b2 Compare July 5, 2024 07:00
@Fintasys
Copy link

I hope we can land this PR soon? It's been open for too long

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants