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

Create widgets level support for States, StateProperties #138270

Closed
Piinks opened this issue Nov 11, 2023 · 6 comments · Fixed by #142151
Closed

Create widgets level support for States, StateProperties #138270

Piinks opened this issue Nov 11, 2023 · 6 comments · Fixed by #142151
Assignees
Labels
Design Systems Study Issues identified during the custom design system study. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team

Comments

@Piinks
Copy link
Contributor

Piinks commented Nov 11, 2023

MaterialStateProperties are really useful for styling components based on their current state
For example, if we consider a button's background color, instead of enumerating all of the possible state combinations that could result in a color...

  • disabled
  • enabled
  • hovered
  • selected
  • disabledAndHovered
  • enabledAndHovered
  • enabledAndHoveredAndSelected
  • ETC, AND SO ON...

You can provide a color based on the current list of states provided to the MaterialStateProperty, so you have evaluate and provide the desired color. For example:

final myColor = MaterialStateProperty.resolveWith<Color?>(
      (Set<MaterialState> states) {
        if (states.contains(MaterialState.pressed) ||
            states.contains(MaterialState.hovered)) {
          return $styles.colors.summer;
        }
        return $styles.colors.white;
      },
    );

This is great, but does not help when developers are building in the widgets library. We should see about moving this concept into the scope of all widgets.

Ideally, we would do this in a non-breaking fashion (preserving MaterialStateProperties), but hopefully in a way that allows the material and non-material versions to be used interchangeably.

@Piinks Piinks added framework flutter/packages/flutter repository. See also f: labels. team-framework Owned by Framework team Design Systems Study Issues identified during the custom design system study. labels Nov 11, 2023
@goderbauer goderbauer added P2 Important issues not at the top of the work list triaged-framework Triaged by Framework team labels Nov 14, 2023
@MitchellGoodwin MitchellGoodwin self-assigned this Jan 17, 2024
auto-submit bot pushed a commit that referenced this issue Mar 19, 2024
Fixes #138270.

Moves the majority of the logic of MaterialStateProperties down to the widgets layer, then has the existing Material classes pull from the widgets versions.
@knopp
Copy link
Member

knopp commented Mar 20, 2024

This is clearly tied to material design. Other widget / design system may have properties that can not be represented by this. For example mixed selection state. I'm not too happy to see material leaking into widgets.

@knopp
Copy link
Member

knopp commented Mar 20, 2024

I'd hope that a change like this would get little bit more consideration. For context, this is the state of our, pretty basic Button (that can however represent correct behavior on desktop platforms):

enum SelectionState {
  on,
  off,
  mixed,
}

class ButtonState {
  ButtonState({
    this.selected = SelectionState.off,
    this.enabled = false,
    this.focused = false,
    this.hovered = false,
    this.pressed = false,
    this.tracked = false,
  });

  /// Determines selection state of the control.
  final SelectionState selected;

  /// Control is enabled and can be interacted with.
  final bool enabled;

  /// Control has keyboard focus.
  final bool focused;

  /// Control is being hovered over by a pointer.
  final bool hovered;

  /// Control is being pressed. When pointer is released without leaving the
  /// control, the control will receive a tap event.
  final bool pressed;

  /// Control is receiving pointer events but the pointer may or may not be
  /// hovering over the control.
  final bool tracked;
}

WidgetStateProperty is not generic enough to represent this. It really doesn't feel right to have something like in the basic widgets package which will cause people to bend their design system to conform to material states.

@Hixie
Copy link
Contributor

Hixie commented Mar 20, 2024

For what it's worth, the argument that convinced me was that while this is not going to apply to all widget libraries, it will apply to some, and we know the current set of states is reasonably fair because it's been stable for a few years.

We don't expect this to be used by all widget libraries, even our own. For example, blankcanvas won't be using it. But it is useful to be in a library common to cupertino and material, and right now that is just widgets.

We don't expect much more to be added that would be common to both, so creating a layer between widgets and cupertino to hold stuff common to cupertino, material, and other libraries that want to use those mechanisms doesn't seem immediately useful and is potentially quite disruptive; however, if the volume of such APIs grows, then that is something we might consider again in the future.

@matthew-carroll
Copy link
Contributor

the argument that convinced me was that while this is not going to apply to all widget libraries, it will apply to some, and we know the current set of states is reasonably fair because it's been stable for a few years.

Isn't this the definition of a counterargument for adding such a thing to the most foundational/fundamental collection of tools in the framework?

Followup question: Given how easily Material details already leak into non-Material widgets (which we all agree shouldn't happen), what's going to prevent this new partial solution to styling from leaking into any number of widgets such that truly arbitrary design systems are no longer convenient, or even possible? I.e., if the Flutter framework developers can't even stop Material from leaking down into a lower level, who is going to ensure that this new style computation system that sits in part of widgets.dart doesn't expand too far in widgets.dart?

If the desire is for a widget styling system that's lower level than Material, but doesn't truly apply to all widgets, why not introduce a new package in between the two? You said such a thing "doesn't seem immediately useful" but that sounds like a greedy algorithm for API design. What do our principles tell us we should do? Seems to me the principles say that this partial solution doesn't belong in the foundational widgets package.

@Hixie
Copy link
Contributor

Hixie commented Mar 20, 2024

the argument that convinced me was that while this is not going to apply to all widget libraries, it will apply to some, and we know the current set of states is reasonably fair because it's been stable for a few years.

Isn't this the definition of a counterargument for adding such a thing to the most foundational/fundamental collection of tools in the framework?

No, not really. We have lots of things in the various layers that aren't used widely by the lower layers. For example I don't think anything in the framework uses BitField (foundation layer) at all, HSLColor and HSVColor (painting layer) aren't used by much if anything either; I can't imagine AnimationMean (animation layer) is used much either... There's lots of examples.

Followup question: Given how easily Material details already leak into non-Material widgets (which we all agree shouldn't happen)

I'm curious to learn more about this. In working on blankcanvas I've found ways in which features that should be more widely available are only defined in cupertino or material (for example, creating a bespoke text field without using material means having to create localizations, magnifiers, etc), but I haven't found that many things that go the other way.

what's going to prevent this new partial solution to styling from leaking into any number of widgets such that truly arbitrary design systems are no longer convenient, or even possible?

In principle that's what code review is for. The team is pretty attuned to the need to make it possible, so I'm not too worried about that. (I would also highly recommend adding such systems to flutter/tests to make sure they continue to be possible.) Personally I'd be more concerned with these API features being adopted by design languages than by them being inconvenient for design languages, because once it seems like the de facto way to do things, libraries that don't use them may be deemed non-idiomatic. That said, my concern that that will happen with this specific API has decreased over time.

If the desire is for a widget styling system that's lower level than Material, but doesn't truly apply to all widgets, why not introduce a new package in between the two?

We considered that (see the discussion on the PR) but eventually concluded that it would be quite disruptive with only minimal benefit. I think if we find the number of things we'd put in such a layer is growing over time, this is something we should reconsider.

You said such a thing "doesn't seem immediately useful" but that sounds like a greedy algorithm for API design.

Greedy algorithm seems like a pretty good first heuristic for API design!

What do our principles tell us we should do? Seems to me the principles say that this partial solution doesn't belong in the foundational widgets package.

I think the most relevant of our principles that applies here is solve real problems. The real problem we wanted to solve was having an API we could use in the cupertino library. Lazy programming also applies to some extent. Both I think point us to the current solution, but of course we should always be ready to refactor should new constraints or requirements emerge.

Copy link

github-actions bot commented Apr 3, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Systems Study Issues identified during the custom design system study. framework flutter/packages/flutter repository. See also f: labels. P2 Important issues not at the top of the work list team-framework Owned by Framework team triaged-framework Triaged by Framework team
Projects
Development

Successfully merging a pull request may close this issue.

6 participants