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

Add support for a new kind of InkSplash: the "ripple" #13986

Merged
merged 7 commits into from Jan 9, 2018

Conversation

HansMuller
Copy link
Contributor

Added the InkRipple InkFeature and an InkSplashType enum to the material library.

Added InkSplashType splashType to material's theme data.

InkWell and InkResponse now have a InkSplashType splashType parameter that determines what kind of ink feature is displayed in reponse to user input. This parameter defaults to Theme.of(..).splashType.

Hoisted color confirm() and cancel() from the InkFeature subclasses to InkFeature.

@Hixie
Copy link
Contributor

Hixie commented Jan 9, 2018

Hoisted color confirm() and cancel() from the InkFeature subclasses to InkFeature.

That won't make sense for the InkDecoration feature.

@Hixie
Copy link
Contributor

Hixie commented Jan 9, 2018

We could introduce an InteractiveInkFeature subclass to avoid the code duplication.

/// gestures (such as tap and long-press) to trigger ink splashes.
/// gestures (such as tap and long-press) to trigger ink splashes. This class
/// is used when the [Theme]'s [ThemeData.splashType] is [InkSplashType.drop]
/// (which is the theme's default splash type).
Copy link
Contributor

Choose a reason for hiding this comment

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

"the theme's default splash type" is unclear; i'd probably just say "which is the default splash type" or some such.

} // else we're probably in deactivate()
}

switch(widget.splashType ?? Theme.of(context).splashType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after switch

final BorderRadius borderRadius = widget.borderRadius ?? BorderRadius.zero;

InkFeature splash;
void onRemoved () {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no space before (

///
/// * [ThemeData.splashType], which defines the splash type for a Material
/// [Theme].
enum InkSplashType {
Copy link
Contributor

Choose a reason for hiding this comment

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

what i wouldn't give for metaclasses.

How about we put a static function on the two classes, each implementing the same signature as the constructors and each just redirecting to the respective constructor, and then instead of this enum we point to one of those functions?

Then if someone wants to make a third kind of splash, it's easy to do.

It's ugly as heck, but InkSplashType is BoxShape all over again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I defined an InteractiveInkFeatureFactory (yow) abstract class that just has a create method. It's the value of InkSplash.splashFactory and InkRipple.splashFactory

_color = value;
controller.markNeedsPaint();
}

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 we should have an abstract class between InkFeature and InkSplash/InkRipple(/InkHighlight?) that hosts these three.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an abstract class called InteractiveInkFeature

Found: center == $center radius == $radius alpha == ${paint.color.alpha}''';
}));

// At this point the splash radius has expanded to its limit: 5 passed the
Copy link
Contributor

Choose a reason for hiding this comment

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

passed -> past

@Hixie
Copy link
Contributor

Hixie commented Jan 9, 2018

LGTM modulo comments

/// The [InkWell] and [InkResponse] widgets generate instances of this
/// class.
abstract class InteractiveInkFeature extends InkFeature {
/// Creates an InteractiveInkFeature.
Copy link
Contributor

Choose a reason for hiding this comment

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

should talk about what arguments have got to be non-null

/// Defines the appearance of ink splashes produces by [InkWell]
/// and [InkResponse].
///
/// See Also
Copy link
Contributor

Choose a reason for hiding this comment

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

See also:

@Hixie
Copy link
Contributor

Hixie commented Jan 9, 2018

LGTM

@HansMuller HansMuller merged commit 03e8ab1 into flutter:master Jan 9, 2018
@HansMuller HansMuller deleted the ripple branch January 9, 2018 22:43
DaveShuckerow pushed a commit to DaveShuckerow/flutter that referenced this pull request May 14, 2018
mono0926 referenced this pull request in mono0926/flutter_bootstrap Jan 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants